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.

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

Add disclaimer to doc template for informational messages clarifying they don't affect score/exit code by default #9171

Closed
alfish2000 opened this issue Oct 20, 2023 · 10 comments · Fixed by #9201
Assignees
Labels
Documentation 📗 Needs PR This issue is accepted, sufficiently specified and now needs an implementation Question

Comments

@alfish2000
Copy link

Question

I'd like to treat messages like I0021 as warnings in my CI system so that the pylint_runner would return a non-zero exit code.
I have enabled useless-suppression in .pylintrc and on the console I get the I0021 message printed properly.
But the pylint_runner still returns 0 as exit code so that the CI job does not detect this as a warning/info/error.
Is there a way to let the pylint_runner return non-zero for e.g. enabled I0021 messages?
Thanks in advance for any help

Documentation for future user

Could be added e.g. here : https://pylint.readthedocs.io/en/latest/user_guide/messages/information/useless-suppression.html#useless-suppression-i0021
Or more generic somewhere where the informational messages like I00xx are explained

Additional context

No response

@alfish2000 alfish2000 added Documentation 📗 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling Question labels Oct 20, 2023
@Pierre-Sassoulas
Copy link
Member

That would be the --fail-on option, or #2293 for a more modular fix. Not sure if we should add a link to this option on all the informational message.

@Pierre-Sassoulas Pierre-Sassoulas added Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Oct 20, 2023
@alfish2000
Copy link
Author

That would be the --fail-on option, or #2293 for a more modular fix. Not sure if we should add a link to this option on all the informational message.

Thanks for your quick answer and the hint to the --fail-on option.

@jacobtylerwalls
Copy link
Member

I can see it being nice to adjust the message page template to add a disclaimer on all the Informational messages that they aren't included in the score by default (and thus won't cause a failure on CI).

@Pierre-Sassoulas Pierre-Sassoulas added Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs decision 🔒 Needs a decision before implemention or rejection labels Oct 21, 2023
@jacobtylerwalls jacobtylerwalls changed the title Treat e.g. I0021 as warning Add disclaimer to doc template for informational messages clarifying they don't affect score/exit code by default Oct 22, 2023
@alfish2000
Copy link
Author

alfish2000 commented Oct 23, 2023

That would be the --fail-on option, or #2293 for a more modular fix. Not sure if we should add a link to this option on all the informational message.

Thanks for your quick answer and the hint to the --fail-on option.

I've tested with --fail-on and pylint then returns with exit code 1.
But the pylint_runner then still returns with exit code 0.
pylint_runner uses "sys.exit(run.linter.msg_status)"
I assume that pylint returns still 0 in its msg_status
Where does this problem then have to be fixed?
Is it a pylint problem? Should pylint set a msg_status != 0 so that pylint_runner would also return with exit code != 0?
I have not debugged anything.
I just had a quick look at the pylint_runner code.
So this is just my assumption for the moment.

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Oct 23, 2023

pylint_runner is maintained by someone else. I'm not sure what the use case is for it.

@alfish2000
Copy link
Author

In pylint in this case there is this special handling in run.py:

            elif linter.any_fail_on_issues():
                # 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.
                sys.exit(self.linter.msg_status or 1)

The pylint_runner just relies on this msg_status.
It does not implement this special handling which pylint has implemented.
pylint_runner is calling:

        run = pylint.lint.Run(self.args + pylint_files, **exit_kwarg)
        sys.stdout = savedout
        sys.stderr = savederr

        sys.exit(run.linter.msg_status)

So my question here for pylint would be if pylint should maybe return non-zero in its msg_status if e.g. --fail-on=useless-suppression is specified.
So this special handling of returning 1 would not be required and as additional benefit a third party app like pylint_runner would just work out of the box without having to implement any additional special handling.

@Pierre-Sassoulas
Copy link
Member

I think the use case for pylint runner does not exists anymore now that the --recursive option was implemented in pylint.

@alfish2000
Copy link
Author

I think the use case for pylint runner does not exists anymore now that the --recursive option was implemented in pylint.

Thanks for pointing me to the --recursive option.

It seems to work so far.
Is there a way to print which files were processed by pylint?
For example with pylint --recursive=y --reports=y . I can see that 53 files are processed but 55 ".py" files are in my directory structure.
So how could I find out which 2 files were not processed?

@Pierre-Sassoulas
Copy link
Member

I'm not sure such an option exists, did you try --verbose / -v ? Next option would be to add a logging here manually:

@alfish2000
Copy link
Author

I'm not sure such an option exists, did you try --verbose / -v ? Next option would be to add a logging here manually:

--verbose did not help

Adding print(self.current_file) here logged all the files.
Thanks for the hint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Needs PR This issue is accepted, sufficiently specified and now needs an implementation Question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants