AI-generated possible solution for #10703 (A)#10924
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Catching a broad
Exceptionaroundself.poetry.package.dependency_group(group_name)will hide unexpected errors; narrow this to the specific exception type that indicates a missing group or check for group existence before calling. - The cleanup logic for empty
group,dependency-groups, andoptional-dependenciessections is now duplicated in multiple branches; consider centralizing this into a small helper or a single post-processing block to avoid divergence in future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching a broad `Exception` around `self.poetry.package.dependency_group(group_name)` will hide unexpected errors; narrow this to the specific exception type that indicates a missing group or check for group existence before calling.
- The cleanup logic for empty `group`, `dependency-groups`, and `optional-dependencies` sections is now duplicated in multiple branches; consider centralizing this into a small helper or a single post-processing block to avoid divergence in future changes.
## Individual Comments
### Comment 1
<location path="src/poetry/console/commands/remove.py" line_range="98" />
<code_context>
)
+ optional_dependencies = project_content.get("optional-dependencies", {})
+ group_sections.extend(
+ (group_name, dependencies, [])
+ for group_name, dependencies in optional_dependencies.items()
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** Use an empty dict instead of a list for the `poetry_section` placeholder when adding optional dependencies.
`group_sections.extend` passes its third element as `poetry_section` into `_remove_packages`, where this parameter is treated as a mapping. Using `[]` here changes its type compared to other call sites and could break membership checks or updates. Use `{}` instead to keep the type consistent and avoid subtle bugs.
</issue_to_address>
### Comment 2
<location path="src/poetry/console/commands/remove.py" line_range="216-218" />
<code_context>
) -> set[str]:
removed = set()
- group = self.poetry.package.dependency_group(group_name)
+ try:
+ group = self.poetry.package.dependency_group(group_name)
+ except Exception:
+ group = None
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching `Exception` around `dependency_group` is broad and may hide real errors.
This blanket `except Exception` will also hide programming errors (e.g. `TypeError`, `AttributeError`), making failures harder to detect. If you only want to ignore missing groups, please catch the specific exception raised when a group is absent (e.g. a Poetry-specific error or `KeyError`) and let other exceptions bubble up. You could also return early from `_remove_packages` when the group is missing instead of using `group = None` and checking later.
Suggested implementation:
```python
removed = set()
try:
group = self.poetry.package.dependency_group(group_name)
except (KeyError, ValueError):
# The requested dependency group does not exist; nothing to remove.
return removed
```
If the rest of `_remove_packages` currently relies on `group` possibly being `None` (e.g. `if group is None: ...`), those branches can now be removed because the early `return removed` handles the “group missing” case. Adjust any such checks accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
| optional_dependencies = project_content.get("optional-dependencies", {}) | ||
| group_sections.extend( | ||
| (group_name, dependencies, []) |
There was a problem hiding this comment.
issue (bug_risk): Use an empty dict instead of a list for the poetry_section placeholder when adding optional dependencies.
group_sections.extend passes its third element as poetry_section into _remove_packages, where this parameter is treated as a mapping. Using [] here changes its type compared to other call sites and could break membership checks or updates. Use {} instead to keep the type consistent and avoid subtle bugs.
| try: | ||
| group = self.poetry.package.dependency_group(group_name) | ||
| except Exception: |
There was a problem hiding this comment.
suggestion (bug_risk): Catching Exception around dependency_group is broad and may hide real errors.
This blanket except Exception will also hide programming errors (e.g. TypeError, AttributeError), making failures harder to detect. If you only want to ignore missing groups, please catch the specific exception raised when a group is absent (e.g. a Poetry-specific error or KeyError) and let other exceptions bubble up. You could also return early from _remove_packages when the group is missing instead of using group = None and checking later.
Suggested implementation:
removed = set()
try:
group = self.poetry.package.dependency_group(group_name)
except (KeyError, ValueError):
# The requested dependency group does not exist; nothing to remove.
return removedIf the rest of _remove_packages currently relies on group possibly being None (e.g. if group is None: ...), those branches can now be removed because the early return removed handles the “group missing” case. Adjust any such checks accordingly.
Hi,
We're a research team at JetBrains studying how maintainers evaluate AI-generated code.
We opened two pull requests as possible solutions to the corresponding issue, each taking a different approach. Both were generated by AI and have not yet been manually reviewed by a human. We’d like to get your perspective on these two solutions via a 10-minute survey. Regardless of your role, we want to know which solution actually saves you work, and which qualities of AI-generated code matter most when working with the codebase.
If you're open to participating in our study, just reply "I'm interested" here, and we'll send the details to the contact specified in your GitHub profile. If you'd rather not take part, feel free to close the PRs – we won’t post them again.
Thanks!
Issue #10703