Repository navigation
refactor: the profile listing's columns move to where dl's rendering is tested - #580
Merged
Merged
Conversation
codecov flagged 65 uncovered lines in `dl/src/commands.rs` on #572, at 0.00%. Both halves of that are true and the second is the interesting one: `commands.rs` has no test module at all, and never has, because it is command plumbing -- dl's rendering lives in `render.rs`, which carries nearly two hundred tests. So 0% there is the house norm rather than a gap. The gap was putting testable decisions in the untested file. Two of them: - **the account column's two absences.** A profile with no credential has nobody to name and renders `-`; one that is logged in with no readable account renders `unknown`, because Claude Code's state file was absent or has moved on from the shape this reads. Neither is an error and neither stops a launch, so that word is the only thing distinguishing them for a reader. - **the shared-account footnote.** Said once per group even though both members carry the note, sorted so the sentence does not reorder itself between runs, and preceded by a blank line so it reads as a footnote rather than a row. `render::claude_profile_lines` returns the lines and `render_claude_profiles` prints them, which is the shape `table_lines` already has for the workspace listing. Nine tests cover the header, the column width against both a long name and one shorter than the header, the three-field join and its gaps, both absences, and the footnote's count, order and blank line. `warn_if_the_profiles_root_could_not_be_read` stays in `commands.rs`, and the line f98170c drew there is the right one: it asks the filesystem a question a pure function cannot answer, and needs a stderr to answer it on. What moves here is only the part that is a function of the rows it was handed. No behaviour change: the output is byte-identical, checked against a fixture profile root before and after.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @JSmithRobotics, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 6 days and 22 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Reviewer's GuideRefactors Claude profile listing formatting out of command plumbing into test-covered rendering helpers while preserving output exactly, and adds focused tests for column layout, account edge cases, empty listings, and shared-account footnotes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…-claude-profiles f60da86 inserted `render_claude_profiles` and its doc *inside* the doc block that already belonged to `render_repos`, which left `render_repos` -- the function that actually reads the completion cache -- with no doc at all, and left its first four lines making a claim about the function below them. The claim is checkable and wrong. "**Divergence row 11 lands here:** a cache document with no `repos` key reads as a cache with no repos" describes row 11 of docs/rust-rewrite-plan.md, the completion-cache row. `render_claude_profiles` touches no cache: it calls `claude_profiles::from_process()` and prints rows. A reader chasing that citation lands on the wrong function and the divergence looks unimplemented. Not a sweep -- e9fbcdc rewrote a paragraph inside this same block, so it is the diff's own comment. Claude-Session: https://claude.ai/code/session_01U4SpX4ixqQG5R8hErsPMot
e9fbcdc replaced "No home directory and no override" with "`dl` runs with `XDG_CACHE_HOME` set and no home directory at all, and then there is no profiles root to enumerate." The second clause is false on an input anyone can name. `DEVLAUNCH_CLAUDE_PROFILES_DIR=/somewhere/empty` with no `$HOME` and no `$CLAUDE_CONFIG_DIR`: `xdg::claude_profiles_root` returns the override at domain/xdg.rs:94 *before* it ever consults `home_dir()`, so there is a profiles root, it is enumerated, and it is empty. `unnamed_config_dir_from_process()` is `None`, so `summarise` builds no default row either and the listing is empty with a root that very much exists. The old wording named the override and was right to. The replacement dropped it and asserted the opposite, which is the failure mode a comment has that code does not: it goes stale and then lies. Says what actually empties the listing now -- no unnamed login to head it, and a root that is absent or empty. Not testable as behaviour: the sentence is byte-identical either way, and what is wrong is the explanation of when it prints. Claude-Session: https://claude.ai/code/session_01U4SpX4ixqQG5R8hErsPMot
"both lived in a file with no test module at all until codecov pointed at 65 uncovered lines on this change" decays twice over. "This change" has no referent to anyone reading the function later, and "a file with no test module at all" stops being true the first time anybody writes one in commands.rs -- at which point the sentence is a confident false claim about a file it does not name. The why survives in the sentence above it, which is the part worth keeping: the column decisions are the interesting part and they are worth a test. The rest is commit-message material and e9fbcdc's message already carries it. Claude-Session: https://claude.ai/code/session_01U4SpX4ixqQG5R8hErsPMot
…from bytes
`claude_profile_lines` measures the name column with `chars().count()`, and the
PR's premise is that the column decisions are worth a test. This one had none:
every name in the suite was ASCII, so `the_name_column_is_as_wide_as_the_widest_name`
and `a_short_name_does_not_shrink_the_column_below_the_header` both pass verbatim
against `row.name.len()`.
Proved by swapping in `len()` and running: green. So the test that goes with this
commit had to be written to fail, and the first attempt did not -- asserting the
header and the rows agree passes under a byte width too, because `{:<width$}` pads
by characters, so a byte width pads every line to the same wrong number and
misaligns nothing. It only opens a gap wider than the longest name.
Asserts the width itself instead. `ünïcødé` is seven characters and eleven bytes,
so the state column starts at 9 with a character count and at 13 with a byte one.
Red under `len()`, green under `chars().count()`.
Claude-Session: https://claude.ai/code/session_01U4SpX4ixqQG5R8hErsPMot
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.
Follow-up to #572, from the one comment on that stack that was worth acting on.
codecov flagged 65 uncovered lines at 0.00% in
dl/src/commands.rs. Bothhalves of that are true, and the second is the interesting one:
commands.rshasno test module at all and never has, because it is command plumbing -- dl's
rendering lives in
render.rs, which carries nearly two hundred tests. So 0%there is the house norm rather than a gap.
The gap was putting testable decisions in the untested file. Two of them, both
described at length in #572's own description and neither covered:
to name and renders
-; one that is logged in with no readable account rendersunknown, because Claude Code's state file was absent or has moved on from theshape this reads. Neither is an error and neither stops a launch, so that word
is the only thing distinguishing them for a reader.
carry the note, sorted so the sentence does not reorder itself between runs, and
preceded by a blank line so it reads as a footnote rather than a row.
render::claude_profile_linesreturns the lines andrender_claude_profilesprints them, which is the shape
table_linesalready has for the workspacelisting. Nine tests cover the header, the column width against both a long name
and one shorter than the header, the three-field join and its gaps, both
absences, and the footnote's count, order and blank line.
What deliberately does not move
warn_if_the_profiles_root_could_not_be_readstays where f98170c put it. The linethat commit drew is the right one: it asks the filesystem a question a pure
function cannot answer and needs a stderr to answer it on, and widening the return
type of
summarisefor a case only the binary can report would be the worsetrade. What moves here is only the part that is a function of the rows it was
handed.
No behaviour change
The output is byte-identical, checked against a fixture profile root before and
after -- including the footnote, and including the
-for a profile with nocredential:
Rebased onto
mainafter the five-PR stack merged, so it carries f98170c'scanonicalised grouping and 485162c's changes rather than the versions those
fixed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AdSFnBdxie6TosHVmjLY28
Summary by Sourcery
Move Claude profile listing presentation into the tested rendering layer without changing its output.
Enhancements:
Tests: