SONARPY-2360 Add unit test case on S5655 when compared types has decorators #2173
Conversation
maksim-grebeniuk-sonarsource
requested a review
from thomas-serre-sonarsource
There was a problem hiding this comment.
LGTM! I left a minor comment to improve the readability of the test.
I don't approve it yet as I'm unsure that the ticket is really matching what you have implementented. Looking at the ticket, it says that the problem is coming from any decorated class. However looking at the test, it seems to me the problem is more specifically dataclasses.
I suggest either enriching the test with other examples of classes with decorators that cause FPs, or updating the tickets (2360 & 2359) to focus on dataclasses specifically
|
|
||
| def fp_decorated_types(): | ||
| import dataclasses | ||
|
|
||
| @dataclasses.dataclass | ||
| class DecoratedClass: | ||
| x : int | ||
|
|
||
| def decorated_type_expecting_foo(data: DecoratedClass): ... | ||
|
|
||
| data = dataclasses.replace(...) | ||
| # FP SONARPY-2359 | ||
| decorated_type_expecting_foo(data) # Noncompliant |
There was a problem hiding this comment.
minor a small readability improvement
| def fp_decorated_types(): | |
| import dataclasses | |
| @dataclasses.dataclass | |
| class DecoratedClass: | |
| x : int | |
| def decorated_type_expecting_foo(data: DecoratedClass): ... | |
| data = dataclasses.replace(...) | |
| # FP SONARPY-2359 | |
| decorated_type_expecting_foo(data) # Noncompliant | |
| import dataclasses | |
| @dataclasses.dataclass | |
| class DecoratedClass: | |
| x : int | |
| def decorated_type_expecting_foo(data: DecoratedClass): ... | |
| data = dataclasses.replace(DecoratedClass(42)) | |
| # FP SONARPY-2359 | |
| decorated_type_expecting_foo(data) # Noncompliant |
There was a problem hiding this comment.
As discussed on another channel, the FP exists for any decorated class. I suggest updating the test to show it more explicitly. Though, I'm not able to propose a working test 🤔
I have added many comments about renaming, feel free to ignore them if you think there are worse than the current naming.
| @@ -267,3 +267,15 @@ class EnumA(Enum): | |||
| C = 3 | |||
|
|
|||
| len(EnumA) | |||
|
|
|||
| def fp_decorated_types(): | |||
There was a problem hiding this comment.
minor as suggested in a previous comment, I really think you can get rid of this line. I was able to raise the FP without this def function
| class DecoratedClass: | ||
| x : int | ||
|
|
||
| def decorated_type_expecting_foo(data: DecoratedClass): ... |
There was a problem hiding this comment.
| def decorated_type_expecting_foo(data: DecoratedClass): ... | |
| def decorated_class_expected(data: DecoratedClass): ... |
| def fp_decorated_types(): | ||
| import dataclasses | ||
| @dataclasses.dataclass | ||
| class DecoratedClass: |
There was a problem hiding this comment.
| class DecoratedClass: | |
| class DecoratedDataClass: |
| class DecoratedClass: | ||
| x : int | ||
|
|
||
| def decorated_type_expecting_foo(data: DecoratedClass): ... |
There was a problem hiding this comment.
| def decorated_type_expecting_foo(data: DecoratedClass): ... | |
| def decorated_type_expecting_foo(data: DecoratedDataClass): ... |
| x : int | ||
|
|
||
| def decorated_type_expecting_foo(data: DecoratedClass): ... | ||
| data = dataclasses.replace(DecoratedClass(42)) |
There was a problem hiding this comment.
| data = dataclasses.replace(DecoratedClass(42)) | |
| data = dataclasses.replace(DecoratedDataClass(42)) |
| data = dataclasses.replace(DecoratedClass(42)) | ||
|
|
||
| # FP SONARPY-2359 | ||
| decorated_type_expecting_foo(data) # Noncompliant |
There was a problem hiding this comment.
| decorated_type_expecting_foo(data) # Noncompliant | |
| decorated_dataclass_expected(data) # Noncompliant |
| @@ -267,3 +267,15 @@ class EnumA(Enum): | |||
| C = 3 | |||
|
|
|||
| len(EnumA) | |||
|
|
|||
There was a problem hiding this comment.
To be more explicit about the scope of the ticket, I suggest to add a more general test case.
I was going to propose this test but I 'm not able to raise the issue with it. Any idea why?
class AnotherClass:
x: int
y: float
@decorator
class DecoratedClass:
x : int
def decorated_class_expected(data: DecoratedClass): ...
data = AnotherClass()
# FP SONARPY-2359
decorated_class_expected(data) # NoncompliantThere was a problem hiding this comment.
cause in this example it compares AnotherClass to be compatible with DecoratedClass and since it has member x as DecoratedClass has - it says that it is compatible type
There was a problem hiding this comment.
Indeed, thanks
maksim-grebeniuk-sonarsource
requested a review
from thomas-serre-sonarsource
maksim-grebeniuk-sonarsource
force-pushed
the
SONARPY-2360
branch
from
a13913c
to
da51d7d
Compare
|
| data = dataclasses.replace(DecoratedDataClass(42)) | ||
|
|
||
| # FP SONARPY-2359 | ||
| decorated_dataclass_expected(data) # Noncompliant |
There was a problem hiding this comment.
| decorated_dataclass_expected(data) # Noncompliant | |
| decorated_dataclass_expected(data) # Noncompliant | |
SONARPY-2360