Enforce AST-based Pattern and Metric interface contracts#814
Conversation
WalkthroughThe changes introduce static typing enhancements across the configuration module, add interface protocols for patterns and metrics, and update related type hints. Tests are refactored to use pytest and introspect callable signatures for correct AST parameter typing. Minor runtime checks and type-checking suppressions are added without altering application logic. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as pytest
participant Config as Config
participant MetricFactory as Metric Factory
participant PatternFactory as Pattern Factory
TestRunner->>Config: get_patterns_config()
Config-->>TestRunner: patterns & metrics configs
loop For each metric in config
TestRunner->>MetricFactory: Inspect value() signature
MetricFactory-->>TestRunner: Signature info
TestRunner->>TestRunner: Assert 'ast' parameter and type
end
loop For each pattern in config
TestRunner->>PatternFactory: Inspect value() signature
PatternFactory-->>TestRunner: Signature info
TestRunner->>TestRunner: Assert 'ast' parameter and type
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
AntonProkopyev
changed the title
AntonProkopyev
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
AntonProkopyev
changed the title
AntonProkopyev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
aibolit/config.py (1)
147-147: Address the type ignore comment when the typing issue is resolved.Based on the past review comment mentioning "Ignore until astral-sh/ty#154", this appears to be a temporary workaround for a known typing limitation.
🧹 Nitpick comments (1)
aibolit/model/model.py (1)
180-180: Consider addressing the underlying typing issues instead of suppressing type checking.While the
@no_type_checkdecorator provides a quick workaround, it would be better to resolve the underlying type checking issues to maintain type safety throughout the codebase.Can you clarify what specific typing issues are causing problems in this method? This would help determine if there's a better solution than suppressing type checking entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
aibolit/__main__.py(2 hunks)aibolit/config.py(6 hunks)aibolit/model/model.py(2 hunks)test/config/test_config.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
aibolit/__main__.py (1)
aibolit/config.py (2)
Config(107-240)Metric(88-90)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: latexmk
- GitHub Check: make
🔇 Additional comments (6)
aibolit/config.py (2)
75-79: Well-defined protocol for Pattern interface.The Protocol definition clearly establishes the expected interface for patterns, requiring a
valuemethod that accepts an AST and returns a list of integers (line numbers).
87-91: Well-defined protocol for Metric interface.The Protocol definition clearly establishes the expected interface for metrics, requiring a
valuemethod that accepts an AST and returns an integer.aibolit/__main__.py (2)
36-36: Good addition of Metric protocol import for runtime validation.This enables the runtime assertion to verify that metric instances conform to the expected interface.
299-299: Excellent runtime validation of protocol compliance.The assertion ensures that factory-created metric objects actually implement the expected
Metricprotocol, providing both static and runtime type safety.aibolit/model/model.py (1)
4-4: Import added for type checking suppression.The
no_type_checkimport enables the decorator used on thetestmethod below.test/config/test_config.py (1)
10-23: Excellent test structure and documentation.The test design using
@pytest.mark.xfailwith detailed TODO comments is a great approach for tracking progress toward full interface compliance. The comprehensive documentation explains both the requirements and next steps clearly.
AntonProkopyev
changed the title
|
@AntonProkopyev Hello team member, I noticed that your branch name |
|
@yegor256 please take a look here |
|
@ivanovmg what do you think? |
|
|
||
| @typing.runtime_checkable | ||
| class Pattern(typing.Protocol): | ||
| def value(self, ast: AST) -> list[int]: |
There was a problem hiding this comment.
@yegor256 @AntonProkopyev in my opinion, having to pass ast (or filepath) into value is not the best idea.
The reason for this is that it makes a class essentially a function.
class SomePattern:
def __init__(self) -> None:
pass # why would we even need a class without an instance variable?
def value(self, ast: AST) -> list[LineNumber]:
# here is a pure function, which does not have any state (ast is not even an attribute)Either we have a class with ast passed as a parameter:
class SomePattern:
def __init__(self, ast: AST) -> None:
self.ast = ast
@classmethod
def from_filepath(cls, filepath: str | os.PathLike) -> SomePattern:
# an alternative constructor, which is easy to use for the end user
ast = ... # create ast from the filepath
return cls(ast)
def value(self) -> list[LineNumber]:
...Or we have a simple function:
def some_pattern(ast: AST) -> list[LineNumber]:
...I personally gravitate towards a class with ast as an input parameter, since it allows for an alternative constructor via a classmethod.
But still, a functional approach may work: there is a library variants, which enables function overloading https://github.com/python-variants/variants. However, the project hasn't been updated for quite a while.
Example:
matching_lines = some_pattern(ast)
matching_lines = some_pattern.from_filepath(filepath)There was a problem hiding this comment.
@ivanovmg (cc: @yegor256) thank you for review! I agree that the "function"-like interface is not a best idea. I think before any refactorings we need to provide clear and obvious contracts between modules and subsystems. After that making the code consistent. And then making it beautiful.
Let me show the steps of refactorings with pseudocode:
- Initial state - unclear contracts, code will brakes
class P1: # works
def value(ast: AST) -> list[LineNumber]: ...
class P2: # wrong
def value(filepath: str) -> list[LineNumber]: ...- Involve the contract which show me as a developer how all patterns should look, all new patterns matches the contract because of tests:
class Pattern(Protocol):
def value(ast: AST) -> list[LineNumber]: ...
class P1: # works
def value(ast: AST) -> list[LineNumber]: ...
class P2: # wrong # type: ignore
def value(filepath: str) -> list[LineNumber]: ...- Fix the rest of patterns
class P1(Pattern): # works
def value(ast: AST) -> list[LineNumber]: ...
class P2(Pattern): # now works
def value(ast: AST) -> list[LineNumber]: ...- After that maybe we realize that we need to provide not only ast, also a code, filename, etc and need get not only a line numbres, but also columns. We involving a new contract:
class Source:
def ast() -> AST: ...
def code() -> str: ...
def name() -> str: ...
class Findings:
def locations() -> Iterable[Location]: ...
class PatternV2(Protocol):
def findings_in(soruce: Source) -> Findings: ...
class P1(Pattern, PatternV2): # supports a new contract
def value(ast: AST) -> list[LineNumber]: ...
def findings_in(soruce: Source) -> Findings: ...
class P2(Pattern):
def value(ast: AST) -> list[LineNumber]: ...- Support the new contract everywhere in code:
class P1(Pattern, PatternV2): ...
class P2(Pattern, PatternV2): ...- Remove the obsolete contract:
class P1(PatternV2): ...
class P2(PatternV2): ...- Finally rename
PatternV2->Pattern
class P1(Pattern): ...
class P2(Pattern): ...In conclusion with this approach we can incrementally fix the entire codebase with small PRs minimizing the risk of braking the code. The current PR is a second step of proposed approach. What do you think?
There was a problem hiding this comment.
@AntonProkopyev I appreciate the way you think of a potential future transition towards Source and Findings! This indeed can be a probable outcome of the project evolution, in my opinion. It is a good point, and I haven't thought of this.
My question still stands. What if we avoid passing ast and/or filepath into value method?
class Pattern(Protocol):
def value(self) -> list[LineNumber]: ...
class ConcretePattern:
def __init__(self, ast: AST) -> None:
self.ast = ast
def value(self) -> list[LineNumber]:
return ...I mean, Pattern is an object for a particular AST, right? Otherwise, it will become a class with static methods, and can be replaced with a function.
Or do you think of some parameters to be injected into Pattern instance apart from ast?
I can understand, that metric can have a tuning parameters, like (max_limit, or something, to throw an error when exceeded), but still even in this case making it be composed out of AST and some tuning parameters still seems a better option to me from the object thinking standpoint.
Then the sequence of the changes proposed from your side will be quite similar:
class Source:
def ast() -> AST: ...
def code() -> str: ...
def name() -> str: ...
class Findings:
def locations() -> Iterable[Location]: ...
class PatternV2(Protocol):
def findings(self) -> Findings: ...
class P1(Pattern, PatternV2): # supports a new contract
def value(self) -> list[LineNumber]: ...
def findings(self) -> Findings: ...
class P2(Pattern):
def value(self) -> list[LineNumber]: ...@yegor256 what do you think from the object perspective regarding Pattern and Metric? Should they be composed out of AST or should they take AST as a parameter into instance methods (which will look more like static methods, IMHO)?
There was a problem hiding this comment.
@ivanovmg I agree, we can use a single Pattern instance per AST instance, and move an ast paramete to __init__ method but this change requires patching of all patterns and control code - bloats the current PR.
Probably we should make another PR with chain of changes proposed - moving ast to constructor.
On the other hand there are patterns with hyperparameters, like P20, but we can bind a parameters with a lambda.
There was a problem hiding this comment.
@AntonProkopyev sure, let's proceed with your suggestion. I wonder how much we will have to change in future to adhere to the new interface though :)
ivanovmg
left a comment
ivanovmg
left a comment
There was a problem hiding this comment.
@yegor256 @AntonProkopyev I think it is good idea that the interface get formalized.
But I suppose we need to put more thought in this before we start making drastic changes to the whole code base.
Here are my thoughts on the Pattern interface in particular.
ivanovmg
left a comment
ivanovmg
left a comment
There was a problem hiding this comment.
Let's proceed with this, keeping in mind incremental changes towards better design, like having ast as a part of Pattern as an instance attribute.
|
@yegor256 сan you please merge the PR? |
|
@rultor merge |
|
@ivanovmg Great job on the code review! 🎉 You've earned +12 points, which is the base reward for reviewing someone else's contribution. Your running balance is now +256. Keep up the good work and remember that additional points can be earned based on the number of hits-of-code and comments in the review, as per our work policy. |
|
@AntonProkopyev Thanks for your contribution! 🚀 You've earned a solid +19 points: +16 base, +7 for your 141 hits-of-code, and a small -4 deduction due to 20 review comments (our policy caps at 8). Keep up the great work and focus on quality to maximize your rewards. Your current balance stands at +33. Remember, contributions under 20 hits-of-code incur an 8-point deduction, so aim for impactful changes! |
5 participants
#813
This PR formalizes the expected interface contract by:
Mypytype hints to explicitly declare parameter and return types.inspectto validate function signatures at runtime.Benefits
Impact
config/maindictionary usage.Summary by CodeRabbit
Refactor
Tests
Style