Skip to content

ls: add --sort-by FIELDS option#434

Open
Copilot wants to merge 4 commits into
mainfrom
copilot/add-sort-by-fields-option
Open

ls: add --sort-by FIELDS option#434
Copilot wants to merge 4 commits into
mainfrom
copilot/add-sort-by-fields-option

Conversation

Copilot AI commented May 28, 2026

Copy link
Copy Markdown
Contributor

con-duct ls returned results in filesystem-glob order with no way to sort. This adds --sort-by to sort output by one or more fields.

Changes

  • cli.py: New --sort-by FIELD [FIELD ...] argument accepting any field from LS_FIELD_CHOICES
  • ls.py: Sorting applied on raw (pre-format) data so numeric fields (e.g. wall_clock_time) sort correctly; runs before --reverse so the two flags compose naturally
  • test_ls.py: Updated existing Namespace fixtures; added test_ls_sort_by and test_ls_sort_by_reverse

Usage

con-duct ls --sort-by prefix
con-duct ls --sort-by command prefix
con-duct ls --sort-by wall_clock_time --reverse   # descending

Copilot AI linked an issue May 28, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Add --sort-by FIELDS option to ls command ls: add --sort-by FIELDS option May 28, 2026
Copilot AI requested a review from yarikoptic May 28, 2026 13:11
@yarikoptic yarikoptic added the semver-minor Increment the minor version when merged label May 28, 2026

@yarikoptic yarikoptic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also just parametrize the test for having reverse -- avoid duplication !!!

Comment thread src/con_duct/cli.py Outdated
nargs="+",
metavar="FIELD",
choices=LS_FIELD_CHOICES,
help=f"Sort results by one or more fields. Available choices: {', '.join(sorted(LS_FIELD_CHOICES))}.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just refer to --fields help like with do in other places -- no need to duplicate all

Comment thread src/con_duct/ls.py Outdated
Comment thread src/con_duct/ls.py Outdated

sort_by = getattr(args, "sort_by", None)
if sort_by:
flat_data = [_flatten_dict(d) for d in run_data_raw]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no need for temp store -- just do map in zip below?

Comment thread src/con_duct/cli.py Outdated
@@ -456,6 +456,13 @@ def _create_ls_parser() -> argparse.ArgumentParser:
action="store_true",
help="List entries in reverse order (most recent first).",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this 'recent' should be removed here since apparently recency seems might be not corresponding.

@yarikoptic yarikoptic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left comments and I thought I already submitted a review also asking for test to become parametrized and not duplicating

@yarikoptic

Copy link
Copy Markdown
Member

@copilot also pay attention to pre-commit.ci CI runs

Copilot AI commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

@copilot also pay attention to pre-commit.ci CI runs

Fixed in the latest commit. Changes made:

  • --sort-by help text now refers to --fields instead of duplicating the choices list (fixes the black line-length issue)
  • Walrus operator used: if sort_by := getattr(args, "sort_by", None):
  • Eliminated the flat_data temp variable by using map() inline in the zip()
  • Removed "most recent first" from --reverse help
  • Sort-by tests are now a single @pytest.mark.parametrize("reverse", [False, True]) standalone function

@yarikoptic yarikoptic marked this pull request as ready for review May 28, 2026 15:42
Copilot AI review requested due to automatic review settings May 28, 2026 15:42
@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.10%. Comparing base (f074bb4) to head (e364fdd).
⚠️ Report is 25 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-by argument to the ls subcommand (restricted to LS_FIELD_CHOICES).
  • Apply sorting in ls() on raw (pre-format) run data so numeric fields sort numerically, then apply --reverse.
  • Update/extend ls tests to include the new sort_by argument 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.

Comment thread src/con_duct/ls.py
Comment on lines +235 to +247
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
),
)
Comment thread test/test_ls.py
Comment on lines +397 to +431
@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)
Comment thread src/con_duct/ls.py
Comment on lines +234 to 251

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please clarify if this would work given the question I gave above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ls: --sort-by FIELDS

3 participants