-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feature: poetry show json output format #10487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: poetry show json output format #10487
Conversation
Reviewer's GuideThis PR adds a new Sequence diagram for poetry show command with --output optionsequenceDiagram
actor User
participant CLI as Poetry CLI
participant ShowCmd as ShowCommand
participant Output as OutputFormatter
User->>CLI: poetry show [options]
CLI->>ShowCmd: parse arguments
ShowCmd->>ShowCmd: validate --output value
alt Invalid --output
ShowCmd->>CLI: print error and exit
else --output=json and --tree
ShowCmd->>CLI: print error and exit
else --output=json
ShowCmd->>Output: serialize package(s) to JSON
Output-->>ShowCmd: JSON string
ShowCmd->>CLI: print JSON output
else --output=text
ShowCmd->>Output: format package(s) as text
Output-->>ShowCmd: text output
ShowCmd->>CLI: print text output
end
Class diagram for ShowCommand with --output optionclassDiagram
class ShowCommand {
+name: str
+description: str
+options: list
+handle() int
+_display_single_package_information()
+_display_packages_information()
}
ShowCommand o-- OutputFormatter
class OutputFormatter {
+serialize_to_json()
+format_as_text()
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
7942b4c to
c1522c0
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Aearsears - I've reviewed your changes - here's some feedback:
- A lot of the JSON vs text test scaffolding is very repetitive—consider extracting a helper or fixture to DRY up the parameterized tests and assertions.
- In show.py you import
jsoninside multiple blocks; moving it to the module scope would remove duplicate imports and improve readability. - There’s a typo in the
--treeerror message ('the the text output'); you might want to correct it to something like 'can only be used with the text output.'
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- A lot of the JSON vs text test scaffolding is very repetitive—consider extracting a helper or fixture to DRY up the parameterized tests and assertions.
- In show.py you import `json` inside multiple blocks; moving it to the module scope would remove duplicate imports and improve readability.
- There’s a typo in the `--tree` error message ('the the text output'); you might want to correct it to something like 'can only be used with the text output.'
## Individual Comments
### Comment 1
<location> `src/poetry/console/commands/show.py:135` </location>
<code_context>
+
+ return 1
+
+ if self.option("output") != "text" and self.option("tree"):
+ self.line_error(
+ "<error>Error: --tree option can only be used the the text output.</error>"
</code_context>
<issue_to_address>
Typo in error message: 'the the text output'.
Please correct the error message to remove the duplicate 'the'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
"<error>Error: --tree option can only be used the the text output.</error>"
=======
"<error>Error: --tree option can only be used with the text output.</error>"
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/poetry/console/commands/show.py:397` </location>
<code_context>
+ if show_why:
+ required_by = reverse_deps(locked, locked_repository)
+ if required_by:
+ content = ",".join(required_by.keys())
+ package["required_by"] = content
+
</code_context>
<issue_to_address>
Joining required_by keys with ',' may be ambiguous if package names contain commas.
Consider outputting 'required_by' as a list rather than a comma-separated string to avoid ambiguity if package names include commas.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if required_by:
content = ",".join(required_by.keys())
package["required_by"] = content
=======
if required_by:
package["required_by"] = list(required_by.keys())
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/console/commands/test_show.py
Outdated
| ) | ||
|
|
||
| tester.execute("--latest") | ||
| tester.execute("--latest" if not output_format else "--latest " + output_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Swap if/else branches of if expression to remove negation (swap-if-expression)
| tester.execute("--latest" if not output_format else "--latest " + output_format) | |
| tester.execute("--latest " + output_format if output_format else "--latest") |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By swapping the
if and else conditions around wecan invert the condition and make it positive.
tests/console/commands/test_show.py
Outdated
| ) | ||
|
|
||
| tester.execute("--outdated") | ||
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Swap if/else branches of if expression to remove negation (swap-if-expression)
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) | |
| tester.execute("--outdated " + output_format if output_format else "--outdated") |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By swapping the
if and else conditions around wecan invert the condition and make it positive.
tests/console/commands/test_show.py
Outdated
| ) | ||
|
|
||
| tester.execute("--outdated") | ||
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Swap if/else branches of if expression to remove negation (swap-if-expression)
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) | |
| tester.execute("--outdated " + output_format if output_format else "--outdated") |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By swapping the
if and else conditions around wecan invert the condition and make it positive.
tests/console/commands/test_show.py
Outdated
| cachy 0.1.0 0.2.0 Cachy package | ||
| pendulum 2.0.0 2.0.1 Pendulum package | ||
| """ | ||
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use f-string instead of string concatenation (use-fstring-for-concatenation)
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) | |
| tester.execute( | |
| "--outdated" if not output_format else f"--outdated {output_format}" | |
| ) |
tests/console/commands/test_show.py
Outdated
| ) | ||
|
|
||
| tester.execute("--outdated") | ||
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use f-string instead of string concatenation (use-fstring-for-concatenation)
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) | |
| tester.execute( | |
| "--outdated" if not output_format else f"--outdated {output_format}" | |
| ) |
tests/console/commands/test_show.py
Outdated
| cachy 0.1.0 0.2.0 Cachy package | ||
| demo 0.1.1 9cf87a2 0.1.2 9cf87a2 Demo package | ||
| """ | ||
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use f-string instead of string concatenation (use-fstring-for-concatenation)
| tester.execute("--outdated" if not output_format else "--outdated " + output_format) | |
| tester.execute( | |
| "--outdated" if not output_format else f"--outdated {output_format}" | |
| ) |
tests/console/commands/test_show.py
Outdated
| tester.execute( | ||
| "--outdated" | ||
| if not output_format | ||
| else "--outdated --without dev " + output_format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use f-string instead of string concatenation (use-fstring-for-concatenation)
| else "--outdated --without dev " + output_format | |
| else f"--outdated --without dev {output_format}" |
tests/console/commands/test_show.py
Outdated
| cachy 0.1.0 Cachy package | ||
| pendulum 2.0.0 Pendulum package | ||
| """ | ||
| tester.execute("--all" if not output_format else "--all " + output_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use f-string instead of string concatenation (use-fstring-for-concatenation)
| tester.execute("--all" if not output_format else "--all " + output_format) | |
| tester.execute("--all" if not output_format else f"--all {output_format}") |
|
question for the reviewer on the ai bot's comment: On line 257 and then on line 638, it imports |
It should be fine. Unfortunately, there is no documentation about which imports are expensive and should not be done at top level. The only way to tell for sure is to do some measurements: Run |
2c7ff8a to
a1650cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Aearsears - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/poetry/console/commands/show.py:394` </location>
<code_context>
+ if show_why:
+ required_by = reverse_deps(locked, locked_repository)
+ if required_by:
+ content = ",".join(required_by.keys())
+ package["required_by"] = content
+
</code_context>
<issue_to_address>
Joining required_by keys with a comma may cause ambiguity if package names contain commas.
Returning required_by as a list of strings would prevent ambiguity and enhance machine-readability.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if required_by:
content = ",".join(required_by.keys())
package["required_by"] = content
=======
if required_by:
package["required_by"] = list(required_by.keys())
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
for single package its a single json object {
"name": "requests", "version": "2.32.3", "description": "Python HTTP for Humans.",
"dependencies": {"certifi": ">=2017.4.17", "charset-normalizer": ">=2,<4", "idna": ">=2.5,<4", "urllib3": ">=1.21.1,<3"},
"required_by": {"cachecontrol": ">=2.16.0", "requests-toolbelt": ">=2.0.1,<3.0.0"}
}for all the packages it will output a list of objects like [
{"name": "virtualenv", "installed_status": "installed", "version": "20.30.0", "latest_version": "20.32.0", "description": "Virtual Python Environment builder"},
{"name": "zstandard", "installed_status": "installed", "version": "0.23.0", "latest_version": "0.23.0", "description": "Zstandard bindings for Python"}
] |
35b06ad to
1420ff3
Compare
colindean
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for undertaking this!
I add one little nit, a personal preference for Enums over lists of strings for enumerations of valid options. I always gently guide folks toward Enums in these situations.
radoering
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. It looks quite good to me. Just some minor remarks.
8786523 to
95b337e
Compare
Co-authored-by: Randy Döring <[email protected]>
Co-authored-by: Randy Döring <[email protected]>
a26cffa to
218e80f
Compare
|
@Aearsears thank you so very much for picking this up and running with it. This is great work and I can't wait to put it into use. @radoering thanks again for doing what you do! I recently onboarded a team of about 50 people to Poetry :-) |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Pull Request Check List
Resolves: #10451
Adds a
--outputoption to thepoetry showcommand fortextandjsonoutputs.poetry self showis not updated and there is some test duplication intest_show.pybut this PR doesn't update them as I wanted to keep changes as contained as possible.Duplicated test coverage:
test_show_non_dev_with_basic_installed_packagescaptured intest_show_basic_with_group_options --without testtest_show_with_group_onlycaptured intest_show_basic_with_group_options --only {MAIN_GROUP}test_show_with_optional_groupcaptured intest_show_basic_with_group_options --with timeI will open another PR for the duplicated tests.
Summary by Sourcery
Implement an --output flag for poetry show that enables JSON output alongside the existing text format, add validation for invalid or conflicting options, serialize package data to JSON in appropriate code paths, update tests accordingly, and document the new flag.
New Features:
Enhancements:
Documentation:
Tests:
Summary by Sourcery
Add an
--outputflag to thepoetry showcommand to support JSON output alongside text, with validation for invalid formats and conflicting--treeusageNew Features:
--outputoption for thepoetry showcommand to choose between text and JSON formatsEnhancements:
--outputargument and emit errors for unsupported formats or invalid--treecombinationsDocumentation:
--outputoption in the CLI reference with supported formats and constraintsTests:
--outputvalues and JSON/--treeincompatibility errors