Add --sort-by option to ls command for flexible result ordering - #446
Open
yarikoptic wants to merge 12 commits into
Open
Add --sort-by option to ls command for flexible result ordering#446yarikoptic wants to merge 12 commits into
yarikoptic wants to merge 12 commits into
Conversation
… list/dict values
Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
Follow-up on the --sort-by review: - Replace the zip()/map() decorate-sort dance with a `_sort_run_data()` helper which flattens each record once and reads top-to-bottom. - Replace `_make_sort_value()` with `_sort_key()`, which ranks values by type (numbers, text, other, missing). info.json files accumulate across duct versions, so one field can hold a number in one run, a string in another, a list (e.g. `gpu`) in a third and be absent from a fourth -- previously any such mix raised TypeError. Missing values keep sorting last. - Sort strings naturally (digit runs compared as numbers), so schema_version 0.9.0 sorts before 0.10.0 and run2_ before run10_ instead of lexicographically. - Warn when no run provides a requested --sort-by field, instead of silently returning an unaffected order. - Read `args.sort_by` directly rather than via getattr(), matching how the other ls arguments are used. - Spell out in --sort-by help that later fields break ties, that runs missing a field are listed last and that --reverse gives descending order. Tests: the sort tests no longer assert an output is sorted relative to itself (which passed even when --sort-by was ignored) -- inputs are now deliberately unordered and the exact expected order is asserted. Added coverage for numeric-not-lexical ordering, natural version ordering, multi-field tie-breaking, mixed/missing values, the missing-field warning, unit tests for _sort_key/_natural_chunks, and argparse tests for --sort-by. The per-LS_FIELD_CHOICES parametrization now feeds numeric values to the numeric fields and displays the sorted field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sEmfsHTr9Ki44PLwzEz8W
Schema 0.2.3 (#425) backfills the OS/distro provenance fields into the "system" block of older records, so the --sort-by test fixtures now write a "system" key -- without it every record older than 0.2.3 fails to load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sEmfsHTr9Ki44PLwzEz8W
yarikoptic
added
the
semver-minor
Claude
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
+ Coverage 91.32% 91.94% +0.62%
==========================================
Files 15 15
Lines 1280 1329 +49
Branches 173 183 +10
==========================================
+ Hits 1169 1222 +53
+ Misses 77 71 -6
- Partials 34 36 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds flexible multi-key sorting to the con-duct ls command via a new --sort-by CLI option, enabling stable ordering across heterogeneous info.json schemas (including natural sorting for version-like strings) while preserving existing --reverse behavior.
Changes:
- Added
--sort-by FIELD [FIELD ...]parsing/validation to thelssubcommand parser. - Implemented type-aware total-order sort keying (including natural chunking for strings) and applied sorting before formatting/field restriction.
- Added extensive test coverage for sorting behavior, mixed types, missing fields, and CLI parsing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/con_duct/cli.py |
Adds --sort-by argument to ls parser and updates --reverse help text. |
src/con_duct/ls.py |
Implements _natural_chunks, _sort_key, _sort_run_data, and integrates sorting into ls(). |
test/test_cli.py |
Tests --sort-by parsing defaults, multi-arg parsing, and invalid-choice handling. |
test/test_ls.py |
Adds helper utilities and comprehensive sorting tests (single/multi-field, natural, numeric, mixed, missing, warnings). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A field can be present in every record and null in all of them -- "gpu" is written as null whenever the machine has no GPU -- so "No run provides" was inaccurate for the case a user is most likely to hit. Both cases are equally a no-op for ordering and equally worth reporting, so keep warning for both and word it for both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sEmfsHTr9Ki44PLwzEz8W
From an independent review of the sort keys: - A digit run longer than sys.get_int_max_str_digits() (4300 by default) made int() raise ValueError, killing the whole `ls` invocation. Free form fields (message, command, ...) hold whatever the user typed, so this was reachable; digit runs too long to be a meaningful number are now compared as text. - Ties were broken by the order the paths were given, which is arbitrary glob order when they were globbed -- two runs of the same command could come out in either order. prefix, the only field guaranteed unique, now breaks any remaining tie. - ensure_compliant_schema() backfills fields added in later schema versions with "", which ranked as text and sorted such runs *ahead* of runs that have a real value. "" now counts as no value, like a missing key or a null, so it sorts last and triggers the same warning. - NaN compares false against everything, which left the order dependent on the input order; it is treated as no value too. Also correct the --sort-by help: valueless runs sort last, but --reverse flips that, so it does not simply "list them last". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sEmfsHTr9Ki44PLwzEz8W
Member
Author
tested also on drogon as in original issuethat it does sort if asked by prefix (noted annoying traceback from pyout... we might want to address here or there even... likely here) |
`con-duct ls | head` ended in a BrokenPipeError traceback out of pyout's final write, and the same holds for the json/yaml/summaries paths -- any of them can be writing when the reader goes away. Catch BrokenPipeError around the subcommand and exit 141 (what a shell reports for a command killed by SIGPIPE), pointing stdout at /dev/null first so that Python's flush during shutdown does not raise a second BrokenPipeError and print a traceback anyway, as the Python docs on SIGPIPE recommend. Verified with 10MB of `ls` output whose reader exits immediately: before, exit 1 with a 19 line traceback ending in pyout/tabular.py; after, exit 141 and nothing on stderr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sEmfsHTr9Ki44PLwzEz8W
Collaborator
|
Thanks for running it on drogon. Took the pyout traceback here, in It is not pyout-specific — the Verified against ~10 MB of Two tests cover it: the exit code with no traceback, and the Say the word if you would rather keep this PR to Generated by Claude Code |
The line handing the subcommand's return code to sys.exit() was never exercised -- every test calling main() either stopped at the help/usage path or, since the previous commit, took the BrokenPipeError branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sEmfsHTr9Ki44PLwzEz8W
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
--sort-bycommand-line option to thelscommand that allows users to sort results by one or more fields. This enables flexible ordering of duct run records beyond the default input order or simple reverse chronological sorting.Key Changes
--sort-byoption to_create_ls_parser()that accepts one or more field names fromLS_FIELD_CHOICES_sort_run_data(): Sorts raw run records by specified fields before formatting/display_sort_key(): Converts field values to comparable sort keys, handling mixed types (numbers, strings, lists, dicts, None) that may appear in the same field across different runs_natural_chunks(): Implements natural/semantic sorting for version strings and numeric components within text (e.g., "run2" < "run10")--fields(not displayed)Implementation Details
--sort-byfields are applied in order, with later fields breaking ties from earlier ones--reverseflag for descending orderTests Added
Comprehensive test coverage including:
https://claude.ai/code/session_017sEmfsHTr9Ki44PLwzEz8W