Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/con_duct/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

)
parser.add_argument(
"--sort-by",
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

)
return parser


Expand Down
15 changes: 15 additions & 0 deletions src/con_duct/ls.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,21 @@ def ls(args: argparse.Namespace) -> int:
)
info_files = [path for path in args.paths if is_info_file(path)]
run_data_raw = load_duct_runs(info_files, args.eval_filter)

sort_by = getattr(args, "sort_by", None)
if sort_by:
Comment thread
yarikoptic marked this conversation as resolved.
Outdated
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?

run_data_raw = [
item
for _, item in sorted(
zip(flat_data, 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 on lines +235 to +247
]

output_rows = process_run_data(run_data_raw, args.fields, formatter)

Comment on lines +234 to 251

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.

if args.reverse:
Expand Down
41 changes: 41 additions & 0 deletions test/test_ls.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ def _run_ls(
format=fmt,
func=ls,
reverse=False,
sort_by=None,
)
buf = StringIO()
with contextlib.redirect_stdout(buf):
Expand Down Expand Up @@ -255,6 +256,7 @@ def test_ls_with_filter(self) -> None:
format="summaries",
func=ls,
reverse=False,
sort_by=None,
)
result = self._run_ls(paths, "summaries", args)

Expand Down Expand Up @@ -383,9 +385,48 @@ def test_ls_reverse(self) -> None:
format="json",
func=ls,
reverse=True,
sort_by=None,
)
result_reversed = self._run_ls(paths, "json", args)
parsed_reversed = json.loads(result_reversed)
prefixes_reversed = [row["prefix"] for row in parsed_reversed]

assert prefixes_reversed == list(reversed(prefixes_normal))

def test_ls_sort_by(self) -> None:
"""Test --sort-by flag sorts entries by the specified field."""
paths = ["file1_info.json", "file2_info.json"]

args = argparse.Namespace(
paths=[os.path.join(self.temp_dir.name, path) for path in paths],
colors=False,
fields=["prefix", "schema_version"],
eval_filter=None,
format="json",
func=ls,
reverse=False,
sort_by=["prefix"],
)
result = self._run_ls(paths, "json", args)
parsed = json.loads(result)
prefixes = [row["prefix"] for row in parsed]
assert prefixes == sorted(prefixes)

def test_ls_sort_by_reverse(self) -> None:
"""Test --sort-by combined with --reverse gives descending order."""
paths = ["file1_info.json", "file2_info.json"]

args = argparse.Namespace(
paths=[os.path.join(self.temp_dir.name, path) for path in paths],
colors=False,
fields=["prefix", "schema_version"],
eval_filter=None,
format="json",
func=ls,
reverse=True,
sort_by=["prefix"],
)
result = self._run_ls(paths, "json", args)
parsed = json.loads(result)
prefixes = [row["prefix"] for row in parsed]
assert prefixes == sorted(prefixes, reverse=True)