job-list: misc cleanup #5144
job-list: misc cleanup #5144
Conversation
|
re-pushed with an extra commit from something I just noticed in the JobList class. I cannot find evidence that we've ever accepted "all" as a filter name. And "all" in the code only sets pending and running, not pending, running, and inactive. So it doesn't make much sense. I suspect its some left over code from development of the class? Perhaps there is something I just can't remember or some use case I can't find. But there's a cleanup commit in here for the time being. |
|
@chu11 I'm going to set MWP here in a bit unless I hear otherwise, we're trying to see if this mergify issue is affecting all PRs our just some... |
|
doh, looks like it hit us again |
|
Ok, we might have to ask the folks at mergify :-( |
|
https://github.com/Mergifyio rebase |
✅ Branch has been successfully rebased |
|
https://github.com/Mergifyio rebase |
Problem: The function list_inactive_cb() was removed a long time ago but the prototype for it is still in list.h. Remove the function prototype.
Problem: There is a comment describing job list filtering with the 'since' filtering. The comment is a little confusing because it does not match the code, doesn't describe how a list is sorted, and happens to use the word "since" in the description. Clarify the comment with more details.
Problem: Some tabbing was incorrect. Make tabbing good!
Problem: The wording in the --since option in flux-jobs(1) is a bit misleading, suggesting that the --since option may apply only to completed/inactive jobs. Solution: Note that the --since option applies to jobs that were active after the given timestamp, which makes it clear that --since also will list pending and running jobs.
Problem: In a test in t2800-jobs-cmd.t, both the --filter and --all option are specified, leading to unexpected output. The test happens to pass, but may not behave as intended. Remove the -a option in the test.
Problem: A comment in t2260-job-list.t is out of date. flux_job_list() has been deprecated and is no longer used. Update the comment.
Problem: The t2250-job-archive.t test does not test that active jobs are not incorrectly archived in the archive database. Add an additional test to double check for this.
Problem: The JobList class's add_filter() function checks for the special filter name "all" and sets the filter to pending and running. As far as I can tell, the special filter name "all" has never been supported or documented. In addition, it doesn't make much sense since it should set pending, running, and inactive to get all jobs. My suspicion is this may be left over code from development that did not get removed. Remove check for the filter name "all".
✅ Branch has been successfully rebased |
Codecov Report
@@ Coverage Diff @@
## master #5144 +/- ##
==========================================
- Coverage 83.13% 83.12% -0.01%
==========================================
Files 453 453
Lines 77788 77784 -4
==========================================
- Hits 64666 64659 -7
- Misses 13122 13125 +3
|
|
This is approved with MWP so I'll merge since mergify still now working. |
This is some misc cleanup / doc updates peeled off from another branch. It also include cleanup in
flux-job, testsuite, andflux-jobs(1).