Skip to content

Improve 'sysand info' command output#292

Open
Jonas-Puksta-Sensmetry wants to merge 1 commit intomainfrom
jp/improve-info
Open

Improve 'sysand info' command output#292
Jonas-Puksta-Sensmetry wants to merge 1 commit intomainfrom
jp/improve-info

Conversation

@Jonas-Puksta-Sensmetry
Copy link
Copy Markdown
Collaborator

Make 'sysand info' print the info of only the highest package version, and print usages better (being more explicit about skipped standard library usages).
Resolves #208.

…, and print usages better

Signed-off-by: jonas.puksta.sensmetry <jonas.puksta@sensmetry.com>
@consideRatio
Copy link
Copy Markdown
Collaborator

Oh nice picking this up @Jonas-Puksta-Sensmetry!!

I'm on mobile and asked opus 4.7 to review with this prompt:

#292 (comment)

Task three Staff SWE to review the linked PR separately. Let one focus on avoiding bugs etc, let another focus on architecture decisions, and another one on code comprehension (comments, naming, etc). Then summarize for me.

(I havn't considered this PR or the AI review myself yet)

Got the diff — single-file change to sysand/src/commands/info.rs (+51/-21). Here's what each reviewer flagged:

Reviewer 1 — Bugs & correctness

  • Behavioral regression: if no project in the URI response has a valid semver version, the command prints nothing to stdout (only a log::warn!). Old code would have printed all of them. A user running sysand info <uri> may see silent success.
  • Warning spam: the loop logs "Version X is not a valid semantic version" once per invalid entry — potentially noisy on large resolvers.
  • Tie-breaking: uses > not >=, so the first project at the max version wins. If do_info ordering isn't deterministic, output becomes non-deterministic on ties.
  • The three new return; statements inside pprint_interchange_project are safe today (usages block is last), but brittle — any future field printed after usages will be silently skipped for the "empty / all-ignored" cases.

Reviewer 2 — Architecture

  • Selection logic leaks into the CLI layer. "Pick highest semver" belongs in sysand_core (or a helper), not inlined in command_info_uri. Makes it untestable and creates divergence with command_info_path, which has no such selection.
  • Asymmetry between command_info_path (prints one) and command_info_uri (now filters to one) deserves a comment or consolidation — why does only the URI variant need picking?
  • The "Some usages are ignored" / "All usages are ignored" strings mix UX with data. Fine for now, but a structured output mode (JSON) will need to rethink this.
  • No tests added for version-selection edge cases (empty, all-invalid, ties, mixed valid/invalid). This logic is subtle enough to warrant a few unit tests.
  • Good call on &InterchangeProjectInfoRaw — removes needless moves and enables the selection loop to hold references cheaply.

Reviewer 3 — Comprehension (naming, comments, clarity)

  • best_version_info is vague ("best" how?). highest_version_project or latest_project reads better.
  • is_usage_ignored is singular but means "any usage is ignored" → rename any_usages_ignored / has_ignored_usages.
  • String inconsistency: "No usages." has a period, the two new messages don't.
  • log::warn!("No semantic versions found") is ambiguous — sounds like nothing resolved. Prefer: "No projects with valid semantic versions".
  • Invalid-version warning doesn't identify which project — add the name/IRI.
  • usages_to_print.iter() on a Vec<&Usage> gives &&Usage; just iterate the vec directly.
  • No doc comment explaining why "highest semver" is the selection policy — worth one line.

Summary / what I'd push back on before merging
The change does what the PR says, and the refactor to borrow &InterchangeProjectInfoRaw is clean. Three things are worth addressing before approval: (1) the silent-stdout case when every version fails semver parsing is a real behavioral regression — either fall back to printing something or surface a proper error; (2) move version selection into core with unit tests; (3) the naming/message polish from reviewer 3 is cheap and noticeably improves the command's UX. The rest (ties, warning volume, early-return brittleness) are nits worth a comment but not blockers.

@consideRatio
Copy link
Copy Markdown
Collaborator

Screenshot_20260417-164944

Also the same AI...

@victor-linroth-sensmetry
Copy link
Copy Markdown
Collaborator

If you're tidying up the info printer, you could also consider colorizing some of the output.

C.f. Cargo
Screenshot 2026-04-22 at 13 25 30

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better sysand info output for multiple projects/versions

3 participants