Clone msg regostry checkstrformat #14
Conversation
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors how error messages related to string formatting are managed within MyPy. The primary goal is to centralize these messages by moving them from direct string literals in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/refacto-bot |
WalkthroughCentralizes format-string error messages into message_registry constants, updates checkstrformat to use them, refines MessageBuilder.fail to accept either strings or ErrorMessage instances via overloads, and tightens typing of two public error codes to Final[ErrorCode]. No control-flow changes in format checking; minor branching added in fail. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Checker as checkstrformat
participant MB as MessageBuilder
participant ER as Errors.report
Checker->>MB: fail(ErrorMessage, context)
Note right of MB: New: accepts ErrorMessage<br/>Extracts msg.value and msg.code
MB->>ER: report(message: str, code: ErrorCode, ...)
ER-->>MB: recorded
MB-->>Checker: None
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (2)mypy/checkstrformat.py (3)
mypy/messages.py (3)
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. Comment |
Clone msg regostry checkstrformatTL;DR: Migrates string formatting error messages from inline strings to centralized message registry constants. Refacto PR SummaryRefactors string formatting error messages in mypy to use the message registry pattern instead of inline strings. Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant CF as checkstrformat.py
participant MB as MessageBuilder
participant MR as message_registry.py
participant EC as errorcodes.py
CF->>MR: Reference error message constant
MR->>EC: Use error code constant
CF->>MB: Call fail(message_registry.ERROR_CONSTANT, ctx)
MB->>MB: Extract message string and code
MB->>MB: Report error with extracted values
Testing GuideClick to expand
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![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
This pull request refactors error messages for string formatting checks by moving them from mypy/checkstrformat.py to mypy/message_registry.py. The MessageBuilder.fail method is now overloaded to accept ErrorMessage objects, which encapsulate both the message and its error code. This is a good refactoring that improves maintainability and centralizes error messages. The implementation is correct and the changes are consistent across the codebase. I have no suggestions for code improvements. As minor points, the PR title has a typo ("regostry" -> "registry") and the PR description could be filled out to summarize the change. Overall, this is a great change.
Code Review: Error Message Registry Refactoring👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| def fail( | ||
| self, | ||
| msg: str | message_registry.ErrorMessage, | ||
| context: Context | None, | ||
| *, | ||
| code: ErrorCode | None = None, | ||
| file: str | None = None, | ||
| allow_dups: bool = False, | ||
| secondary_context: Context | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
Missing Type Overload
The fail method implementation accepts both str and ErrorMessage types, but the second overload signature is missing the 'code' parameter. This inconsistency could lead to runtime errors when code is provided with an ErrorMessage parameter.
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- ISO-IEC-25010-Reliability-Maturity
| message_registry.UNRECOGNIZED_FORMAT_SPEC.format(spec.format_spec[1:]), | ||
| call, | ||
| code=codes.STRING_FORMATTING, | ||
| ) |
There was a problem hiding this comment.
Missing Error Code
The error code parameter has been removed when migrating to message registry, but the message registry constant should include this code. All other migrations maintain the error code through the message registry constant.
Standards
- Logic-Verification-Data-Flow
- Business-Rule-Consistency
- Algorithm-Correctness-Pattern-Matching
| FORMAT_STR_INVALID_SPECIFIER: Final = ErrorMessage( | ||
| "Invalid conversion specifier in format string", codes.STRING_FORMATTING | ||
| ) |
There was a problem hiding this comment.
Message Naming Convention
Error message constants use inconsistent naming patterns. Some messages use verb-noun format (e.g., FORMAT_STR_INVALID_SPECIFIER) while others use noun-adjective format. Standardizing naming conventions would improve maintainability by making message identifiers more predictable.
Standards
- Clean-Code-Naming-Conventions
- Maintainability-Quality-Consistency
| @@ -290,10 +279,40 @@ def fail( | |||
| file: str | None = None, | |||
| allow_dups: bool = False, | |||
| secondary_context: Context | None = None, | |||
| ) -> None: | |||
| ... | |||
|
|
|||
| @overload | |||
| def fail( | |||
| self, | |||
| msg: message_registry.ErrorMessage, | |||
| context: Context | None, | |||
| *, | |||
| file: str | None = None, | |||
| allow_dups: bool = False, | |||
| secondary_context: Context | None = None, | |||
| ) -> None: | |||
| ... | |||
There was a problem hiding this comment.
Overload Method Complexity
The overloaded fail() method introduces complexity with duplicate parameter lists. Consider using a single method with a union type (str | ErrorMessage) and type narrowing instead of overloads to reduce duplication and improve maintainability.
Standards
- Clean-Code-Functions
- SOLID-DRY
- Maintainability-Quality-Complexity
| message_registry.UNRECOGNIZED_FORMAT_SPEC.format(spec.format_spec[1:]), | ||
| call, | ||
| code=codes.STRING_FORMATTING, | ||
| ) |
There was a problem hiding this comment.
Missing Error Code
The code parameter has been removed from this error message call but not replaced with a message registry entry that includes the code. This could lead to inconsistent error reporting and missing error codes in output.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Completeness
2 participants
(Explain how this PR changes mypy.)
Summary by CodeRabbit
New Features
Refactor