Dynamic Pagination in CPT Dashboard #135
Conversation
|
And now that the reformatting is merged ... you've got a bunch of conflicts to resolve here. 😆 Once you've rebased and resolved the conflicts, make sure that you run |
MVarshini
force-pushed
the
PANDA-633_1
branch
from
b2642d4 to
2a086b0
Compare
There was a problem hiding this comment.
And now that you've merged the black reformatting, I recommend that you make a habit of running black before pushing any commit to a PR. At some point we'll be able to turn on black --check in the CI so we won't accidentally break the conventions -- but even then, doing it proactively is a good idea. (You can turn on git pre-commit hooks if you want, though I've found that can sometimes become annoying and I prefer to do it manually when I'm ready.)
There was a problem hiding this comment.
A few new comments, but they're more style suggestions; and one remaining unanswered confusion about your logic at #135 (comment); but this is looking good.
| if len(jobs) == 0: | ||
| return {"data": jobs, "total": response["total"]} | ||
|
|
||
| jobs[["group"]] = jobs[["group"]].fillna(0) | ||
| jobs.fillna("", inplace=True) | ||
| if len(jobs) == 0: | ||
| return jobs | ||
| return jobs | ||
|
|
There was a problem hiding this comment.
It would reduce the code complexity if you just included those two lines in a len(jobs) != 0 case instead of having an identical return early.
There was a problem hiding this comment.
There are a lot of potential refactoring ideas we should pursue to make the somewhat messy codebase more maintainable.
Right now, we've got a long list of PRs we'd really like to land, and we need to focus on the most critical concerns ... starting with landing this PR, #138, and then a follow-on filtering PR which ought to complete the "revamp" branch. Once we land that onto "main" we can churn through my backlog of ilab PRs.
At that point, our focus should be cleaning up the code base (including unit testing, functional testing, lint and format checkers) to make it more maintainable with a viable CI.
As much as I hate being "pragmatic" in things like this, let's not let minor stuff like this bog us down at this point. Maybe leaving these comments in place and open as a reference for later work isn't a bad idea ...
| jbs = cleanJobs | ||
| jbs["shortVersion"] = jbs["ocpVersion"].str.slice(0, 4) | ||
|
|
||
| return jbs | ||
| return {"data": jbs, "total": response["total"]} |
There was a problem hiding this comment.
What does cleanJobs come from? It may benefit from a comment.
And I do not think jbs makes sense as a variable name. It's just confusing and looks like a typo. Dave also questioned this.
There was a problem hiding this comment.
cleanJobs is the result of a pandas dataframe filter to remove rows, although my understanding of the complicated pandas infrastructure is minimal.
What bugs me here is the jbs = cleanJobs to effectively just obscure the name before returning the data using jbs. That's not new with Varshini's changes, though, and I don't think it's practical to push her to rewrite more existing logic than necessary for the revamp. (On the other hand, simply replacing cleanJobs with jbs or dropping the intermediary jbs would make the code easier to read, and it'd be "simple"... it's just that an endless list of "simple" things isn't simple anymore. 🤔 💣 )
There was a problem hiding this comment.
We will definitely need to keep a note of this for a future refactor PR.
| ) | ||
| df["version"] = df["version"].apply( | ||
| lambda x: x if len(x.split(":")) == 1 else x.split(":")[1][:7] | ||
| import pandas as pd |
There was a problem hiding this comment.
Is there a reason the comment for the function has an import between it and the function?
There was a problem hiding this comment.
Yeah, the import should be at the top. I'm wondering whether isort (which I intent to run when we move into code ownership/cleanup after our PRs are merged) will fix this??? 🤔 In any case, it's a new import and ought to be moved up to the top section.
There was a problem hiding this comment.
Moved the imports to the top
| if len(jobs) == 0: | ||
| return {"data": jobs, "total": response["total"]} | ||
|
|
||
| jobs[["group"]] = jobs[["group"]].fillna(0) | ||
| jobs.fillna("", inplace=True) | ||
| if len(jobs) == 0: | ||
| return jobs | ||
| return jobs | ||
|
|
There was a problem hiding this comment.
There are a lot of potential refactoring ideas we should pursue to make the somewhat messy codebase more maintainable.
Right now, we've got a long list of PRs we'd really like to land, and we need to focus on the most critical concerns ... starting with landing this PR, #138, and then a follow-on filtering PR which ought to complete the "revamp" branch. Once we land that onto "main" we can churn through my backlog of ilab PRs.
At that point, our focus should be cleaning up the code base (including unit testing, functional testing, lint and format checkers) to make it more maintainable with a viable CI.
As much as I hate being "pragmatic" in things like this, let's not let minor stuff like this bog us down at this point. Maybe leaving these comments in place and open as a reference for later work isn't a bad idea ...
| jbs = cleanJobs | ||
| jbs["shortVersion"] = jbs["ocpVersion"].str.slice(0, 4) | ||
|
|
||
| return jbs | ||
| return {"data": jbs, "total": response["total"]} |
There was a problem hiding this comment.
cleanJobs is the result of a pandas dataframe filter to remove rows, although my understanding of the complicated pandas infrastructure is minimal.
What bugs me here is the jbs = cleanJobs to effectively just obscure the name before returning the data using jbs. That's not new with Varshini's changes, though, and I don't think it's practical to push her to rewrite more existing logic than necessary for the revamp. (On the other hand, simply replacing cleanJobs with jbs or dropping the intermediary jbs would make the code easier to read, and it'd be "simple"... it's just that an endless list of "simple" things isn't simple anymore. 🤔 💣 )
| ) | ||
| df["version"] = df["version"].apply( | ||
| lambda x: x if len(x.split(":")) == 1 else x.split(":")[1][:7] | ||
| import pandas as pd |
There was a problem hiding this comment.
Yeah, the import should be at the top. I'm wondering whether isort (which I intent to run when we move into code ownership/cleanup after our PRs are merged) will fix this??? 🤔 In any case, it's a new import and ought to be moved up to the top section.
|
|
||
|
|
There was a problem hiding this comment.
Now that the import is removed, should this gap still be here?
There was a problem hiding this comment.
Interesting question. Do we need two blank lines here? Did this get run through black again? If not, it would probably be a good idea before we merge this.
| jbs = cleanJobs | ||
| jbs["shortVersion"] = jbs["ocpVersion"].str.slice(0, 4) | ||
|
|
||
| return jbs | ||
| return {"data": jbs, "total": response["total"]} |
There was a problem hiding this comment.
We will definitely need to keep a note of this for a future refactor PR.
|
I tested the Pr locally and gone through the changes, I think we are good to go, let's wait also for @vishnuchalla to take an extra look and test. |
|
Few observations while navigating through the dashboard.
|
If we want to take these improvements/bug fixes in follow up PRs and get us unblocked for now, we can get this PR merged into |
|
Thanks for the review and testing, @vishnuchalla. (Now we need to encapsulate all that testing into an automated CI ... but we'll get there.) @MVarshini already has one more PR lined up on the All three of the issues you found need to be fixed on revamp before we drop to main and stage it, but not necessarily in this PR unless we can point out changed code that's responsible. Since #138 focuses on backend sorting, it's likely to touch these issues only by accident, but I don't remember the exact scope of the remaining PR so it's possible there's some overlap already? |
|
I created new issues to record @vishnuchalla's three problems on the REVAMP branch. We should re-evaluate these after @MVarshini's remaining two PRs are stabilized, and fix them if still necessary before dropping I'm going to go ahead and merge this PR so she can move forward with #135. |
Type of change
Description
The
[PROD]_JOBS_APIacceptssizeandoffsetparamsRelated Tickets & Documents
Checklist before requesting a review
Testing