Feature: Add --with-groups flag to show groups for each direct dependency#10687
Closed
ammargit93 wants to merge 7 commits intopython-poetry:mainfrom
Closed
Feature: Add --with-groups flag to show groups for each direct dependency#10687ammargit93 wants to merge 7 commits intopython-poetry:mainfrom
ammargit93 wants to merge 7 commits intopython-poetry:mainfrom
Conversation
Reviewer's GuideAdds a --with-groups flag to Sequence diagram for poetry show with --with-groups flag handling and group mappingsequenceDiagram
actor User
participant CLI
participant ShowCommand
participant GroupCommand
participant ProjectPackage
User->>CLI: run poetry show --with-groups [--tree]
CLI->>ShowCommand: instantiate and parse options
CLI->>ShowCommand: handle()
alt with-groups and tree
ShowCommand-->>ShowCommand: option("with-groups") and option("tree")
ShowCommand->>CLI: line_error("Error: Cannot use --tree and --with-groups at the same time.")
ShowCommand-->>CLI: return 1
else with-groups without tree
ShowCommand-->>ShowCommand: _display_packages_information(...)
ShowCommand-->>ShowCommand: show_with_groups = option("with-groups")
ShowCommand->>ShowCommand: dep_group_map()
alt dep_group_map not cached
ShowCommand->>GroupCommand: build_dependency_group_map()
GroupCommand->>ProjectPackage: project_with_activated_groups_only()
ProjectPackage-->>GroupCommand: root
GroupCommand-->>GroupCommand: map main and group dependencies
GroupCommand-->>ShowCommand: dependency_group_map
ShowCommand-->>ShowCommand: cache _dep_group_map
else dep_group_map cached
ShowCommand-->>ShowCommand: reuse _dep_group_map
end
loop for each locked package
ShowCommand->>ShowCommand: get_groups_from_package(locked.pretty_name)
ShowCommand-->>ShowCommand: lookup in dep_group_map
ShowCommand-->>ShowCommand: attach groups to package info
end
ShowCommand-->>CLI: formatted output with groups column and JSON groups field
end
Class diagram for updated ShowCommand and GroupCommand with dependency group mappingclassDiagram
class GroupCommand {
+_group_dependency_options() list_Option
+_validate_group_options(group_options dict_str_set_str) void
+build_dependency_group_map() dict_str_str
+project_with_activated_groups_only() ProjectPackage
}
class EnvCommand
class ShowCommand {
+name : str
+description : str
-_dep_group_map : dict_str_str
+handle() int
+_display_packages_information(locked_repository Repository, root ProjectPackage) int
+_display_packages_tree_information(locked_repository Repository, root ProjectPackage) int
+get_groups_from_package(package_name str) str
+dep_group_map() dict_str_str
}
GroupCommand <|-- ShowCommand
EnvCommand <|-- ShowCommand
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The groups mapping is rebuilt multiple times inside
_display_packages_information(and again inget_groups_from_package), which is unnecessary work; consider building the dependency→groups map once per command invocation and reusing it for both width calculation and rendering. get_groups_from_packagereturns the sentinel string"-"and the caller then splits/compares against it for JSON output, which couples formatting with data; it would be cleaner to have this helper return a list of groups (or[]/None) and let the caller handle display formatting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The groups mapping is rebuilt multiple times inside `_display_packages_information` (and again in `get_groups_from_package`), which is unnecessary work; consider building the dependency→groups map once per command invocation and reusing it for both width calculation and rendering.
- `get_groups_from_package` returns the sentinel string `"-"` and the caller then splits/compares against it for JSON output, which couples formatting with data; it would be cleaner to have this helper return a list of groups (or `[]`/`None`) and let the caller handle display formatting.
## Individual Comments
### Comment 1
<location> `src/poetry/console/commands/show.py:349-351` </location>
<code_context>
),
)
+ if show_with_groups:
+ group_map = self.build_dependency_group_map()
+ max_len = max((len(v) for v in group_map.values()), default=0)
+ groups_length = max_len
+
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recomputing the dependency group map and max length inside the loop.
These values are recalculated on every iteration and in both branches of the `if installed_package` conditional. Since the group mapping doesn’t depend on `locked`, build it once before iterating over `locked_packages` and derive `groups_length` from it there. This avoids unnecessary work and keeps the loop logic simpler, especially with large dependency graphs.
Suggested implementation:
```python
name_length = version_length = latest_length = required_by_length = (
groups_length
) = 0
latest_packages = {}
latest_statuses = {}
installed_repo = InstalledRepository.load(self.env)
group_map = {}
if show_with_groups:
group_map = self.build_dependency_group_map()
groups_length = max((len(v) for v in group_map.values()), default=0)
),
)
```
```python
```
1. Ensure that `show_with_groups` is defined *before* the new precomputation block (e.g., typically as `show_with_groups = self.option("with-groups")` or similar). If it is currently defined after `installed_repo = InstalledRepository.load(self.env)`, move its definition above that line.
2. If the loop body relies on `group_map` (e.g., accessing `group_map[package]`), no change is needed because it is now defined once above; just confirm there is no other shadowing or redefinition of `group_map` inside the loop.
</issue_to_address>
### Comment 2
<location> `src/poetry/console/commands/show.py:383` </location>
<code_context>
if self.option("format") == OutputFormats.JSON:
packages = []
+ groups = self.build_dependency_group_map()
for locked in locked_packages:
</code_context>
<issue_to_address>
**issue:** The `groups` map built here is never used and gets shadowed later.
In the JSON branch, `groups` is initialized with `build_dependency_group_map()` but then overwritten in the loop with the string from `get_groups_from_package`, so the original dict is never used. Either pass the precomputed map into `get_groups_from_package`/use it directly, or remove this initialization to avoid dead code and extra work.
</issue_to_address>
### Comment 3
<location> `src/poetry/console/commands/show.py:514-516` </location>
<code_context>
else:
line += " " * required_by_length
+ if write_groups:
+ groups = self.get_groups_from_package(locked.pretty_name)
+ line += " " * (groups_length - len(groups)) + groups
+
if write_description:
</code_context>
<issue_to_address>
**suggestion:** Guard against negative padding when `groups_length` is smaller than the string length.
When `groups_length` is 0 or less than `len(groups)`, `(groups_length - len(groups))` is negative, so the padding becomes `""` and the column is no longer fixed-width. Using `max(0, groups_length - len(groups))` makes the intent explicit and avoids misaligned output in these edge cases.
```suggestion
if write_groups:
groups = self.get_groups_from_package(locked.pretty_name)
padding = max(0, groups_length - len(groups))
line += " " * padding + groups
```
</issue_to_address>
### Comment 4
<location> `src/poetry/console/commands/show.py:536-540` </location>
<code_context>
return 0
+ def get_groups_from_package(self, package_name):
+ dep_map = self.build_dependency_group_map()
+ if package_name in dep_map:
</code_context>
<issue_to_address>
**suggestion (performance):** `get_groups_from_package` rebuilds the dependency group map on every call.
This calls `build_dependency_group_map()` for every package and is used inside loops over `locked_packages` for both table and JSON output. Since the mapping comes from static pyproject data, compute it once (e.g. cache on the instance or pass it into `get_groups_from_package`) to avoid repeated parsing and improve performance with many dependencies.
```suggestion
def get_groups_from_package(self, package_name):
# Lazily build and cache the dependency group map so it is only
# constructed once per command invocation instead of on every call.
if not hasattr(self, "_dependency_group_map"):
self._dependency_group_map = self.build_dependency_group_map()
dep_map = self._dependency_group_map
if package_name in dep_map:
return dep_map[package_name]
return "-"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider keeping
dep_group_mapasdict[str, list[str]]instead of a comma-joinedstr, so you don't need to join/split for different outputs and can avoid string parsing in_display_packages_information. build_dependency_group_maprelies on the privateroot._dependency_groupsattribute; if there is any public API for accessing dependency groups, prefer that to reduce coupling to internal implementation details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider keeping `dep_group_map` as `dict[str, list[str]]` instead of a comma-joined `str`, so you don't need to join/split for different outputs and can avoid string parsing in `_display_packages_information`.
- `build_dependency_group_map` relies on the private `root._dependency_groups` attribute; if there is any public API for accessing dependency groups, prefer that to reduce coupling to internal implementation details.
## Individual Comments
### Comment 1
<location> `src/poetry/console/commands/group_command.py:142-151` </location>
<code_context>
message_parts.append(f"{group} (via {opts})")
raise GroupNotFoundError(f"Group(s) not found: {', '.join(message_parts)}")
+
+ def build_dependency_group_map(self) -> dict[str, str]:
+ root = self.project_with_activated_groups_only()
+ dep_group_map: dict[str, list[str]] = defaultdict(list)
+ # main dependencies
+ for dep in root.requires:
+ dep_group_map[dep.name].append("main")
+ # dependency groups
+ for group_name, group in root._dependency_groups.items():
+ for dep in group.dependencies:
+ dep_group_map[dep.name].append(group_name)
+
+ return {
+ name: ",".join(sorted(set(groups)))
+ for name, groups in dep_group_map.items()
+ }
</code_context>
<issue_to_address>
**suggestion:** Consider returning a list of groups instead of a comma-joined string from `build_dependency_group_map`.
`build_dependency_group_map` currently returns `dict[str, str]` where the value is a comma-joined list of group names, and some callers then split this string back into a list. This couples data representation to display and adds unnecessary join/split work.
Returning `dict[str, list[str]]` would keep the map as a structured data model, and callers like `_display_packages_information` could handle any string joining needed for display.
Suggested implementation:
```python
def build_dependency_group_map(self) -> dict[str, list[str]]:
root = self.project_with_activated_groups_only()
dep_group_map: dict[str, list[str]] = defaultdict(list)
# main dependencies
for dep in root.requires:
dep_group_map[dep.name].append("main")
# dependency groups
for group_name, group in root._dependency_groups.items():
for dep in group.dependencies:
dep_group_map[dep.name].append(group_name)
return {
name: sorted(set(groups))
for name, groups in dep_group_map.items()
}
```
Callers that assume `build_dependency_group_map` returns a comma-joined `str` must be updated to work with `list[str]` instead. In particular:
1. Any code that currently does something like `groups = group_map[name].split(",")` should be changed to use the list directly (e.g. `groups = group_map[name]`).
2. Any display logic (for example in `_display_packages_information`) that expects a string should explicitly join the list for presentation, e.g. `", ".join(groups)` (possibly sorted, if ordering is important).
3. If this method is part of a public API used outside this module, corresponding type hints and any external callers must be updated to the new `dict[str, list[str]]` return type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ammargit93
force-pushed
the
feat/show-with-groups
branch
from
59f6fcd to
37d1edc
Compare
ammargit93
force-pushed
the
feat/show-with-groups
branch
from
37d1edc to
00e2a06
Compare
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
2 participants
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.
Pull Request Check List
This PR aims to resolve #7519 and is closely related to #8164.
Changes
Added a boolean flag --with-groups to poetry show.
When enabled:
JSON output:
--tree:
output
Add support for displaying dependency groups in
poetry show, including CLI flag handling and output formatting updates.New Features:
--with-groupsflag topoetry showto display the groups for each dependency in tabular and JSON output.pyproject.tomlproject dependencies and dependency groups, exposing group information per package.Enhancements:
--treetogether with--with-groupsby returning a clear error when both flags are supplied.poetry showto account for the new groups column in the human-readable output.Summary by Sourcery
Add support for displaying dependency groups in
poetry show, including a new CLI flag and corresponding output changes.New Features:
--with-groupsflag topoetry showto display the groups for each dependency.poetry show.Enhancements:
--treetogether with--with-groupsby returning a clear error when both flags are supplied.poetry showto account for the new groups column in the tabular output.Documentation:
--with-groupsoption in the CLI reference.