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.
ls: add --sort-by FIELDS option #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
ls: add --sort-by FIELDS option #434
Changes from all commits
a5148d2c34300b68ff39ae364fddFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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) beforeprocess_run_datarestricts to--fields, so all fields are available for sorting regardless of what's displayed. Addedtest_ls_sort_by_non_displayed_field(committest: add test for sorting by non-displayed field) which sorts bycommandwhilefields=["prefix"]omits it from output, and verifies the output order matches sorted command order.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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--fieldsonly happens insideprocess_run_dataon 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_fielddemonstrates this: it sorts bycommandwhilefields=["prefix"]omitscommandfrom the output, and the test verifies the output is incommand-sorted order.Uh oh!
There was an error while loading. Please reload this page.