Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SONARPY-2329 Add tests documenting new S2638 FNs #2159

Merged
merged 2 commits into from
Nov 15, 2024
Merged

SONARPY-2329 Add tests documenting new S2638 FNs #2159

merged 2 commits into from
Nov 15, 2024

Conversation

thomas-serre-sonarsource
Copy link
Contributor

@thomas-serre-sonarsource thomas-serre-sonarsource commented Nov 15, 2024 •
edited by jira bot
Loading

SONARPY-2329

Part of

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 15, 2024
View reviewed changes
...on-checks/src/test/resources/checks/changeMethodContract/changeMethodContractIntermediate.py Outdated
class IntermediateClass(ParentClass):
...

class IntermediateWithAnyClass(ParentClass[Any]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make a difference in case we have an extra file here compared to just having these classes declared in the changeMethodContractParent.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two files are indeed enough to reproduce the problem. I'll correct that.

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 15, 2024
View reviewed changes
...on-checks/src/test/resources/checks/changeMethodContract/changeMethodContractIntermediate.py Outdated
class IntermediateClass(ParentClass):
...

class IntermediateWithAnyClass(ParentClass[Any]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real reason of the FN is not related to the Any type, but related to inheritance from types with generic type. So, I guess it will be easier to understand what's going on if the class name will be like IntermediateWithGenericTypeParent or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource requested changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good, I have two comments whiches need to be clarified

@sonarqube-next SonarQube-Next
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource merged commit 22cf009 into master Nov 15, 2024
12 checks passed
@maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource deleted the ts/SONARPY-2329 branch November 15, 2024 12:24
thomas-serre-sonarsource added a commit that referenced this pull request Nov 18, 2024
@thomas-serre-sonarsource
SONARPY-2329 Add tests documenting new S2638 FNs (#2159)
0f856ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomas-serre-sonarsource @maksim-grebeniuk-sonarsource