fix(output): standardize --output to work for all formats#1193
fix(output): standardize --output to work for all formats#1193shifa-khan wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR standardizes Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/fromager/commands/list_overrides.py (1)
30-37: ⚡ Quick winAdd a
.. versionchanged::note for the new--outputbehavior.This updates user-facing CLI semantics, but there’s no Sphinx version directive documenting the change in the command docs/docstring.
Suggested patch
def list_overrides( @@ ) -> None: - """List all of the packages with overrides in the current configuration.""" + """List all of the packages with overrides in the current configuration. + + .. versionchanged:: X.Y.Z + ``--output`` now applies to plain output (without ``--details``) and writes + package names to the specified file. + """As per coding guidelines,
**/*.{rst,py}: Use Sphinxversionadded,versionremoved,versionchangeddirectives for user-facing changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fromager/commands/list_overrides.py` around lines 30 - 37, The CLI gained a new --output/ -o Click option but the command docstring/docs are missing a Sphinx directive documenting this behavior change; update the command's docstring or the Sphinx documentation for the list_overrides command to include a ".. versionchanged::" entry describing the new --output behavior (mentioning the option name and that output can be written to a file instead of stdout), placing the directive near other CLI option documentation so readers see the change; update the docstring in src/fromager/commands/list_overrides.py (the function/class that defines the command and help text) accordingly.Source: Coding guidelines
src/fromager/commands/package.py (1)
124-125: ⚡ Quick winDocument the
list-versions --outputcontract change with.. versionchanged::.The command’s user-facing behavior changed for
versions/requirements, but there’s no Sphinx version directive capturing it.Suggested patch
def list_versions( @@ ) -> None: """List all available versions for a package requirement specifier. @@ Use --ignore-per-package-overrides to see what the global cooldown policy would block without per-package exemptions. + + .. versionchanged:: X.Y.Z + ``--output`` now applies to plain ``versions`` and ``requirements`` formats. """As per coding guidelines,
**/*.{rst,py}: Use Sphinxversionadded,versionremoved,versionchangeddirectives for user-facing changes.Also applies to: 233-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fromager/commands/package.py` around lines 124 - 125, The public behavior of the list-versions/requirements CLI changed: the --output flag now writes to a file instead of stdout but there is no Sphinx version directive documenting this; add a ".. versionchanged:: <new-version>" entry in the user-facing docs (the RST page(s) that document the list-versions and requirements commands) describing that "list-versions --output" (and requirements --output) now writes output to a file path instead of stdout and showing the new expected usage/format; mirror the same note for the other instance referenced (the change also applies to the block around lines 233-237) so both command docs get the versionchanged directive and a brief example of the new behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_list_overrides.py`:
- Around line 234-237: The test's stdout leak assertions are incomplete: add an
assertion to check that "test-pkg" is not present in result.stdout alongside the
existing checks for "test-other-pkg" and "test-pkg-library" so that any
plain-output leak of "test-pkg" will fail; update the assertions around
result.stdout in tests/test_list_overrides.py to include assert "test-pkg" not
in result.stdout.
In `@tests/test_list_versions.py`:
- Around line 332-348: Update the two tests that write to an output file
(test_list_versions_output_file_versions_format and the similar test for pins
format) to also assert that the CLI produced no plain version/pin data on stdout
when --output/-o is used: after invoking via _invoke_list_versions (which
returns the click CliRunner Result), add an assertion like assert
result.output.strip() == "" (or result.stdout/strip depending on test helper) to
ensure stdout is empty; keep the existing file-content assertions unchanged.
---
Nitpick comments:
In `@src/fromager/commands/list_overrides.py`:
- Around line 30-37: The CLI gained a new --output/ -o Click option but the
command docstring/docs are missing a Sphinx directive documenting this behavior
change; update the command's docstring or the Sphinx documentation for the
list_overrides command to include a ".. versionchanged::" entry describing the
new --output behavior (mentioning the option name and that output can be written
to a file instead of stdout), placing the directive near other CLI option
documentation so readers see the change; update the docstring in
src/fromager/commands/list_overrides.py (the function/class that defines the
command and help text) accordingly.
In `@src/fromager/commands/package.py`:
- Around line 124-125: The public behavior of the list-versions/requirements CLI
changed: the --output flag now writes to a file instead of stdout but there is
no Sphinx version directive documenting this; add a ".. versionchanged::
<new-version>" entry in the user-facing docs (the RST page(s) that document the
list-versions and requirements commands) describing that "list-versions
--output" (and requirements --output) now writes output to a file path instead
of stdout and showing the new expected usage/format; mirror the same note for
the other instance referenced (the change also applies to the block around lines
233-237) so both command docs get the versionchanged directive and a brief
example of the new behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd4a8c25-9993-439f-98d8-d7cab0f72e2c
📒 Files selected for processing (4)
src/fromager/commands/list_overrides.pysrc/fromager/commands/package.pytests/test_list_overrides.pytests/test_list_versions.py
| else: | ||
| print(row["version"]) | ||
| fh: typing.TextIO | ||
| if output: |
There was a problem hiding this comment.
I like the pattern used here, which avoids duplicating the logic to produce the output. Some of the other changes duplicate logic that is simple today but that could change over time.
I recommend you go 1 step further and create a context manager function that takes as input the path or None value and manages the file handle for you. That would eliminate another source of duplication and allow you to do something like
with get_file_handle_or_console(output) as fh:
# logic to produce the output and send it to fh
There was a problem hiding this comment.
@dhellmann Thanks for the feedback. Updated in latest version.
Wire -o into plain formats for `list-versions` (versions, requirements) and `list-overrides` (no --details), removing inconsistent warn-and-ignore behavior. Add `output_file_or_stdout()` context manager to `clickext` to deduplicate file-handle management across export functions. Closes: python-wheel-build#1177 Signed-off-by: shifa-khan <shikhan@redhat.com>
2 participants
Wire -o into plain formats for
list-versions(versions, requirements) andlist-overrides(no --details), removing inconsistent warn-and-ignore behavior. Matches the find-updates reference implementation.Closes: #1177