fix(output): standardize --output to work for all formats#1193
Open
shifa-khan wants to merge 1 commit into
Open
fix(output): standardize --output to work for all formats#1193shifa-khan wants to merge 1 commit into
shifa-khan wants to merge 1 commit into
Conversation
Wire -o into plain formats for `list-versions` (versions, requirements) and `list-overrides` (no --details), removing inconsistent warn-and-ignore behavior. Matches the find-updates reference implementation. Closes: python-wheel-build#1177 Signed-off-by: shifa-khan <shikhan@redhat.com>
📝 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
Comment on lines
+234
to
237
| # stdout should be empty (no data printed to console) | ||
| assert "test-other-pkg" not in result.stdout | ||
| assert "test-pkg-library" not in result.stdout | ||
|
|
There was a problem hiding this comment.
Complete the stdout leak assertion for plain output.
The test misses test-pkg in the “not in stdout” checks, so a partial stdout leak could still pass.
Suggested patch
# stdout should be empty (no data printed to console)
assert "test-other-pkg" not in result.stdout
+ assert "test-pkg" not in result.stdout
assert "test-pkg-library" not in result.stdout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # stdout should be empty (no data printed to console) | |
| assert "test-other-pkg" not in result.stdout | |
| assert "test-pkg-library" not in result.stdout | |
| # stdout should be empty (no data printed to console) | |
| assert "test-other-pkg" not in result.stdout | |
| assert "test-pkg" not in result.stdout | |
| assert "test-pkg-library" not in result.stdout |
🤖 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 `@tests/test_list_overrides.py` around lines 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.
Comment on lines
+332
to
+348
| def test_list_versions_output_file_versions_format( | ||
| cli_runner: CliRunner, tmp_path: pathlib.Path | ||
| ) -> None: | ||
| """--output is ignored with a warning for 'versions' and 'requirements' formats.""" | ||
| """--output writes plain version list to a file for 'versions' format.""" | ||
| output_file = tmp_path / "versions.txt" | ||
| for fmt in ("versions", "requirements"): | ||
| result = _invoke_list_versions( | ||
| cli_runner, | ||
| extra_args=["--format", fmt, "--output", str(output_file)], | ||
| ) | ||
| assert result.exit_code == 0 | ||
| assert not output_file.exists(), ( | ||
| f"--output should be ignored for --format {fmt}" | ||
| ) | ||
| assert "Warning: --output option is ignored" in result.output | ||
| result = _invoke_list_versions( | ||
| cli_runner, | ||
| extra_args=["--format", "versions", "--output", str(output_file)], | ||
| ) | ||
| assert result.exit_code == 0 | ||
| assert output_file.exists() | ||
| content = output_file.read_text() | ||
| lines = [line for line in content.strip().split("\n") if line.strip()] | ||
| assert "1.2.2" in lines | ||
| assert "1.3.2" in lines | ||
| assert "2.0.0" in lines | ||
|
|
There was a problem hiding this comment.
Assert stdout remains data-free when --output is used for plain formats.
These tests only validate file contents. They should also verify the command does not emit plain version/pin data to stdout when -o is set.
Suggested patch
def test_list_versions_output_file_versions_format(
@@
lines = [line for line in content.strip().split("\n") if line.strip()]
assert "1.2.2" in lines
assert "1.3.2" in lines
assert "2.0.0" in lines
+ assert "1.2.2" not in result.stdout
+ assert "1.3.2" not in result.stdout
+ assert "2.0.0" not in result.stdout
@@
def test_list_versions_output_file_requirements_format(
@@
lines = [line for line in content.strip().split("\n") if line.strip()]
assert "test-pkg==1.2.2" in lines
assert "test-pkg==1.3.2" in lines
assert "test-pkg==2.0.0" in lines
+ assert "test-pkg==1.2.2" not in result.stdout
+ assert "test-pkg==1.3.2" not in result.stdout
+ assert "test-pkg==2.0.0" not in result.stdoutAlso applies to: 350-365
🤖 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 `@tests/test_list_versions.py` around lines 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.
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
1 participant
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.
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