Skip to content

Allow failing when score is affected by Information warning #5318

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.

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

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Based on work in #5303

Not really worth a changelog entry imo. Nobody will have run into this problem, but it might be useful for ourselves at some point.

@coveralls
Copy link

coveralls commented Nov 15, 2021

Pull Request Test Coverage Report for Build 1512268538

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0009%) to 93.505%

Totals Coverage Status
Change from base Build 1512227017: 0.0009%
Covered Lines: 13993
Relevant Lines: 14965

πŸ’› - Coveralls

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I think we need to keep the exit_code that can be analyzed bitwise. Ie : https://www.google.com/search?client=firefox-b-d&q=pylint+exit+code (first result, even included in their research page by google). I thought a little about it and I think going to 64 by adding 32 for I message would break a lot of things script and doc and stackoverflow answers included. So I guess the best option would be to consider enabled I message like if they were C messages. Do you think of something else ?

@DanielNoord
Copy link
Collaborator Author

So I guess the best option would be to consider enabled I message like if they were C messages.

This would make the whole point of bit-wise encoding the exit code pointless for I and C messages. I don't think we should do this.

I thought a little about it and I think going to 64 by adding 32 for I message would break a lot of things script and doc and stackoverflow answers included.

Why don't we add I as 64 though? If people are using the exit codes correctly this shouldn't matter. It breaks the consistency, but would work otherwise.

@Pierre-Sassoulas
Copy link
Member

Why don't we add I as 64 though? If people are using the exit codes correctly this shouldn't matter. It breaks the consistency, but would work otherwise.

It's true that the correct use is with % so except for hard coded value (ie "they used the exit code wrong") it should not be a real problem. Fair enough. Also it's the only solution that would bring consistency.

@DanielNoord
Copy link
Collaborator Author

Why don't we add I as 64 though? If people are using the exit codes correctly this shouldn't matter. It breaks the consistency, but would work otherwise.

It's true that the correct use is with % so except for hard coded value (ie "they used the exit code wrong") it should not be a real problem. Fair enough. Also it's the only solution that would bring consistency.

I'll add a commit that 1) changes I to have message code 64 and 2) updates the docs to reflect this!

@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas Thought about it again, but I don't think we can do this. Taking the example from the useless-suppression PR we would then always have an exit code on either 3.6 or 3.10. That might break CI's which use pylint on both versions. This is also why Marc was hesitant about changing this in the first place.

I think the change in this PR is still an option. We do it in another place as well, and if somebody sets the fail-under and the score is below that score it is weird that we don't fail. In this case I think it is better to exit with 1 than not exit at all.

@Pierre-Sassoulas
Copy link
Member

We could design this like mypy and ignore the discrepancies between python interpreters : pylint-dev/astroid#1248 (comment)

sys.exit(0)
else:
# We need to make sure we return a failing exit code in this case.
# So we use self.linter.msg_status if that is non-zero, otherwise we just return 1.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the 1 here. Is it really necessary ? Shouldn't it be 32 because if self.linter.msg_status is 0 then there is only Informational messages ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do the same on L414. 32 corresponds to usage error so that also doesn't make much sense. We don't have a good code for Information..

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, it's another issue entirely.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 8a17bb5 into pylint-dev:main Nov 29, 2021
@DanielNoord DanielNoord deleted the score branch November 29, 2021 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants