ls: add --sort-by FIELDS option#434
Conversation
yarikoptic
left a comment
There was a problem hiding this comment.
also just parametrize the test for having reverse -- avoid duplication !!!
| nargs="+", | ||
| metavar="FIELD", | ||
| choices=LS_FIELD_CHOICES, | ||
| help=f"Sort results by one or more fields. Available choices: {', '.join(sorted(LS_FIELD_CHOICES))}.", |
There was a problem hiding this comment.
just refer to --fields help like with do in other places -- no need to duplicate all
|
|
||
| sort_by = getattr(args, "sort_by", None) | ||
| if sort_by: | ||
| flat_data = [_flatten_dict(d) for d in run_data_raw] |
There was a problem hiding this comment.
I think no need for temp store -- just do map in zip below?
| @@ -456,6 +456,13 @@ def _create_ls_parser() -> argparse.ArgumentParser: | |||
| action="store_true", | |||
| help="List entries in reverse order (most recent first).", | |||
There was a problem hiding this comment.
I wonder if this 'recent' should be removed here since apparently recency seems might be not corresponding.
yarikoptic
left a comment
There was a problem hiding this comment.
I left comments and I thought I already submitted a review also asking for test to become parametrized and not duplicating
|
@copilot also pay attention to pre-commit.ci CI runs |
Fixed in the latest commit. Changes made:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #434 +/- ##
==========================================
- Coverage 91.92% 91.10% -0.82%
==========================================
Files 15 15
Lines 1127 1248 +121
Branches 140 171 +31
==========================================
+ Hits 1036 1137 +101
- Misses 69 77 +8
- Partials 22 34 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds configurable sorting to con-duct ls via a new --sort-by FIELD [FIELD ...] CLI option, enabling users to order listed runs by one or more info fields (with --reverse applied after sorting).
Changes:
- Add
--sort-byargument to thelssubcommand (restricted toLS_FIELD_CHOICES). - Apply sorting in
ls()on raw (pre-format) run data so numeric fields sort numerically, then apply--reverse. - Update/extend
lstests to include the newsort_byargument and add a sorting test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/con_duct/cli.py |
Adds --sort-by CLI flag and updates --reverse help text. |
src/con_duct/ls.py |
Implements sorting of loaded run records prior to formatting/output generation. |
test/test_ls.py |
Updates Namespace fixtures to include sort_by and adds a new sorting test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if sort_by := getattr(args, "sort_by", None): | ||
| run_data_raw = [ | ||
| item | ||
| for _, item in sorted( | ||
| zip(map(_flatten_dict, run_data_raw), run_data_raw), | ||
| key=lambda x: tuple( | ||
| ( | ||
| x[0].get(k) is None, | ||
| x[0].get(k) if x[0].get(k) is not None else "", | ||
| ) | ||
| for k in sort_by | ||
| ), | ||
| ) |
| @pytest.mark.parametrize("reverse", [False, True]) | ||
| def test_ls_sort_by(reverse: bool, tmp_path: Any) -> None: | ||
| """Test --sort-by flag sorts entries by the specified field, with optional reverse.""" | ||
| files = { | ||
| "file1_info.json": { | ||
| "schema_version": MINIMUM_SCHEMA_VERSION, | ||
| "execution_summary": {}, | ||
| "prefix": "test1", | ||
| }, | ||
| "file2_info.json": { | ||
| "schema_version": MINIMUM_SCHEMA_VERSION, | ||
| "execution_summary": {}, | ||
| "prefix": "test2", | ||
| }, | ||
| } | ||
| for filename, content in files.items(): | ||
| path = tmp_path / filename | ||
| path.write_text(json.dumps(content)) | ||
|
|
||
| paths = [str(tmp_path / f) for f in files] | ||
| args = argparse.Namespace( | ||
| paths=paths, | ||
| colors=False, | ||
| fields=["prefix", "schema_version"], | ||
| eval_filter=None, | ||
| format="json", | ||
| func=ls, | ||
| reverse=reverse, | ||
| sort_by=["prefix"], | ||
| ) | ||
| buf = StringIO() | ||
| with contextlib.redirect_stdout(buf): | ||
| assert ls(args) == 0 | ||
| prefixes = [row["prefix"] for row in json.loads(buf.getvalue().strip())] | ||
| assert prefixes == sorted(prefixes, reverse=reverse) |
|
|
||
| if sort_by := getattr(args, "sort_by", None): | ||
| run_data_raw = [ | ||
| item | ||
| for _, item in sorted( | ||
| zip(map(_flatten_dict, run_data_raw), run_data_raw), | ||
| key=lambda x: tuple( | ||
| ( | ||
| x[0].get(k) is None, | ||
| x[0].get(k) if x[0].get(k) is not None else "", | ||
| ) | ||
| for k in sort_by | ||
| ), | ||
| ) | ||
| ] | ||
|
|
||
| output_rows = process_run_data(run_data_raw, args.fields, formatter) | ||
|
|
There was a problem hiding this comment.
this does look much cleaner but would it still have all the fields at that point or only what is to be displayed etc? (not looking at the code ATM)
There was a problem hiding this comment.
@copilot this does look much cleaner but would it still have all the fields at that point or only what is to be displayed etc? (not looking at the code ATM -- do code analysis and try to sort by fields which are not to be displayed... add test for that)
There was a problem hiding this comment.
Yes — the sort operates on run_data_raw (full raw dicts) before process_run_data restricts to --fields, so all fields are available for sorting regardless of what's displayed. Added test_ls_sort_by_non_displayed_field (commit test: add test for sorting by non-displayed field) which sorts by command while fields=["prefix"] omits it from output, and verifies the output order matches sorted command order.
There was a problem hiding this comment.
@copilot please clarify if this would work given the question I gave above
There was a problem hiding this comment.
Yes, it works for non-displayed fields. The sort (lines 235–248) runs on run_data_raw — the full unfiltered dicts from disk — by temporarily flattening them with _flatten_dict. Field restriction to --fields only happens inside process_run_data on line 250, after sorting is complete. So every field in the JSON is available as a sort key regardless of what is selected for display. test_ls_sort_by_non_displayed_field demonstrates this: it sorts by command while fields=["prefix"] omits command from the output, and the test verifies the output is in command-sorted order.
con-duct lsreturned results in filesystem-glob order with no way to sort. This adds--sort-byto sort output by one or more fields.Changes
cli.py: New--sort-by FIELD [FIELD ...]argument accepting any field fromLS_FIELD_CHOICESls.py: Sorting applied on raw (pre-format) data so numeric fields (e.g.wall_clock_time) sort correctly; runs before--reverseso the two flags compose naturallytest_ls.py: Updated existingNamespacefixtures; addedtest_ls_sort_byandtest_ls_sort_by_reverseUsage