Skip to content

SONARPY-751 Rule S5886: Function return types should be consistent with their type hint#825

Merged
guillaume-dequenne merged 4 commits intomasterfrom
SONARPY-751
Aug 10, 2020
Merged

SONARPY-751 Rule S5886: Function return types should be consistent with their type hint#825
guillaume-dequenne merged 4 commits intomasterfrom
SONARPY-751

Conversation

@guillaume-dequenne
Copy link
Copy Markdown
Contributor

No description provided.

@guillaume-dequenne guillaume-dequenne force-pushed the SONARPY-751 branch 7 times, most recently from a40ca18 to 2d67be6 Compare August 6, 2020 15:48
@guillaume-dequenne guillaume-dequenne changed the title SONARPY-751 SONARPY-751 Rule S5886: Function return types should be consistent with their type hint Aug 6, 2020
@guillaume-dequenne guillaume-dequenne force-pushed the SONARPY-751 branch 14 times, most recently from aa107f6 to 690e40e Compare August 7, 2020 13:26
@guillaume-dequenne guillaume-dequenne marked this pull request as ready for review August 7, 2020 13:33
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, I left some minor comments

@Beta
boolean isCompatibleWith(InferredType other);

@Beta
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you add a comment explaining why we're adding this API and how it differs from canBeOrExtend?

Symbol symbol = ((Name) expression).symbol();
if (symbol != null && "typing.Text".equals(symbol.fullyQualifiedName())) {
return InferredTypes.runtimeType(builtinSymbols.get("str"));
if (symbol != null ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

minor: there is an extra space.
Why are we doing this? InferredTypes.runtimeType should already handle the case where symbol is null

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We still branch depending on symbol.fullyQualifiedName() right after so I think we still need the null check.

else:
x = "hello"
return x # OK

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we could add a test case:

  def foo(cond) -> int:
    if cond:
      return 42
    else:
      return "hello"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is a test case similar to this one on line 94-98, which does flag the str return as Noncompliant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ops sorry I missed it

@sonarsource-next
Copy link
Copy Markdown

Kudos, SonarQube Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

98.4% 98.4% Coverage
0.0% 0.0% Duplication

@guillaume-dequenne guillaume-dequenne merged commit e213e58 into master Aug 10, 2020
@guillaume-dequenne guillaume-dequenne deleted the SONARPY-751 branch August 10, 2020 13:29
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request Feb 3, 2026
GitOrigin-RevId: 40db5dc813096f251eb1e54529891ae2544c5040
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants