Better error messaging #806
Conversation
Reviewer's Guide by SourceryThis pull request improves error messaging when a KeyError occurs during package inclusion. It adds a try/except block around the inclusion logic and appends a helpful note to the KeyError exception, suggesting that the issue might stem from using a Poetry v1 Sequence diagram for improved error handling in package inclusionsequenceDiagram
participant User
participant Poetry
participant PackageInclude
User->>Poetry: Run poetry command
activate Poetry
Note over Poetry: Try to process package includes
alt Success
Poetry->>PackageInclude: Create package include
PackageInclude-->>Poetry: Package include created
else KeyError
Poetry->>Poetry: Add note about v1/v2 compatibility
Poetry-->>User: Error with helpful message
end
deactivate Poetry
Class diagram showing updated error handling in Module classclassDiagram
class Module {
-_package_includes: List
-_explicit_includes: List
-_path: Path
+__init__(path, packages, includes)
}
note for Module "Added try/catch block for KeyError
with improved error message"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @RobbertDM - I've reviewed your changes - here's some feedback:
Overall Comments:
- The try-catch block is too broad and could catch unrelated KeyErrors. Consider wrapping only the specific
package["format"]access if that's the intended target. - Please add tests to verify the error message behavior, particularly for the v1/v2 version mismatch scenario.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| except KeyError as e: | ||
| e.add_note("It might be that you are trying to run poetry v2 with a v1 pyproject.toml") | ||
| raise e |
There was a problem hiding this comment.
suggestion: Consider making the error message more specific by including which key was missing
The current message assumes version mismatch, but a KeyError could occur for other reasons. Including the missing key name (available via str(e)) would make debugging easier.
| except KeyError as e: | |
| e.add_note("It might be that you are trying to run poetry v2 with a v1 pyproject.toml") | |
| raise e | |
| except KeyError as e: | |
| e.add_note(f"Missing required key '{str(e)}'. This might occur if you are trying to run poetry v2 with a v1 pyproject.toml") | |
| raise e |
There was a problem hiding this comment.
But it calls add_note, so I believe the whole KeyError message is still there when raising e further?
There was a problem hiding this comment.
You are absolutely right. My apologies! I mistakenly focused on improving the clarity regarding which key was missing and overlooked that the original exception message, containing the key, is preserved. The suggested change is therefore redundant. Thanks for pointing that out! 👍
|
@RobbertDM this might be superseded by #805 |
Resolves: python-poetry#
I just had a Gitlab pipeline fail with
And that's it for the error message.
It failed because of this
formats=package["format"]KeyError, but in the error message it didn't even mention that it was a KeyError. We didn't know it was poetry related.I hope to save a lot of people some time by including this try/catch.
Summary by Sourcery
Bug Fixes:
KeyErroroccurs during package inclusion to help users diagnose the issue faster.