Migrate checkstrformat to use ErrorMessage class
#7
Conversation
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Tagging @hauntsaninja since the |
WalkthroughThis update standardizes and centralizes error message handling for string and bytes formatting errors in the codebase. All explicit error message strings in Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used🪛 Ruff (0.8.2)mypy/checkstrformat.py904-906: Use a single (SIM102) 🔇 Additional comments (23)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/gemini review |
|
/gemini |
1 similar comment
|
/gemini |
|
It looks like you're trying to invoke a Gemini command by including |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request migrates error codes in checkstrformat.py to use ErrorMessage objects stored in message_registry. This change improves the consistency and maintainability of error messages related to string formatting. The use of predefined message constants makes it easier to update and manage error messages in the future. The code changes are well-structured and easy to follow.
Summary of Findings
- Missing Error Code in
UNRECOGNIZED_FORMAT_SPEC: The error messageUNRECOGNIZED_FORMAT_SPECinmessage_registry.pylacks an associated error code. While the other new error messages havecodes.STRING_FORMATTING, this one is missing it, which could lead to inconsistencies in error reporting and filtering.
Merge Readiness
The pull request is well-structured and improves the consistency of error messages. However, the missing error code for UNRECOGNIZED_FORMAT_SPEC should be addressed before merging. I am unable to directly approve this pull request, and recommend that another reviewer also assesses these changes. After addressing the identified issue, the pull request will be ready for merging.
| ): | ||
| # TODO: add support for some custom specs like datetime? | ||
| self.msg.fail( | ||
| "Unrecognized format" ' specification "{}"'.format(spec.format_spec[1:]), | ||
| message_registry.UNRECOGNIZED_FORMAT_SPEC.format(spec.format_spec[1:]), | ||
| call, | ||
| code=codes.STRING_FORMATTING, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Consider adding a code to the fail call, for consistency with the other calls in this file. This will also allow users to filter this error specifically.
self.msg.fail(
message_registry.UNRECOGNIZED_FORMAT_SPEC.format(spec.format_spec[1:])
call,
code=codes.STRING_FORMATTING,
)
continue
A granular reimplementation of python#12004
Changes
checkstrformat.pyto useErrorMessageobjects stored inmessage_registryfailmethod inMessageBuilderto acceptErrorMessageobjects (the overload is similar to the one here).Summary by CodeRabbit
New Features
Refactor
Style