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

Fix inconsistent argument exit code when argparse exit with its own error code #7931

Merged

Conversation

dmrlawson
Copy link
Contributor

@dmrlawson dmrlawson commented Dec 12, 2022

Type of Changes

Type
🐛 Bug fix

Description

We were bitten by this causing us to think that pylint was completing successfully when in fact a typo in a configuration file was causing argparse to exit with exit code 2. This didn't happen in older versions of pylint for whatever reason. The documentation for pylint suggests that this means "error message issued" but that pylint executed properly when this is not the case.

It feels a bit clumsy to just add in try..except, but on the other hand it's perhaps unexpected that argparse will raise a SystemExit. I'm open to any suggestions to improve this.

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.9 milestone Dec 13, 2022
@Pierre-Sassoulas Pierre-Sassoulas changed the title Invalid argument exit code Fix inconsistent argument exit code when argparse exit with its own error code Dec 13, 2022
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.

Nice catch, we indeed missed that argparse could exit with its own exit code and the inconsistency with pylint's error codes. I think we should use -1 because it's the only way our bit encoded error code can be extended to 64 if required. Thoughts @DanielNoord ?

@dmrlawson
Copy link
Contributor Author

Nice catch, we indeed missed that argparse could exit with its own exit code and the inconsistency with pylint's error codes. I think we should use -1 because it's the only way our bit encoded error code can be extended to 64 if required. Thoughts @DanielNoord ?

sys.exit(32) appears throughout the code. The "usage error" exit code could be factored out to be a constant and then changed but it's a bit more than I was intending here.

$ grep -r 'sys.exit(32)'
pylint/pyreverse/utils.py:        sys.exit(32)
pylint/pyreverse/utils.py:        sys.exit(32)
pylint/lint/run.py:            sys.exit(32)
pylint/lint/run.py:            sys.exit(32)
pylint/lint/run.py:            sys.exit(32)
pylint/lint/run.py:                sys.exit(32)
pylint/config/arguments_manager.py:            sys.exit(32)
pylint/config/config_initialization.py:        sys.exit(32)
pylint/config/config_initialization.py:            sys.exit(32)

@github-actions

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member

You're right let's keep it as 32, I wasn't up to date on this aspect of the code sorry.

DanielNoord
DanielNoord previously approved these changes Dec 14, 2022
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.

Could you fix the tests (there's some other test that is somewhat fragile you'll need to add the new file to it) and add a changelog entry please ?

…t in a config file

This is confusing behaviour. The output is:

```
usage: pylint [options]
pylint: error: ambiguous option: --no could match --notes, --notes-rgx, --no-docstring-rgx
```

The exit code is 2 which doesn't match the documentation: https://pylint.pycqa.org/en/latest/user_guide/usage/run.html#exit-codes
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.

Great job 👌

doc/whatsnew/fragments/7931.bugfix Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) December 14, 2022 22:08
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3699121501

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 95.476%

Totals Coverage Status
Change from base Build 3698112388: 0.001%
Covered Lines: 17663
Relevant Lines: 18500

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas merged commit 62232b3 into pylint-dev:main Dec 14, 2022
github-actions bot pushed a commit that referenced this pull request Dec 14, 2022
…rror code (#7931)

Returning 2 here is confusing as it doesn't match the documentation: https://pylint.pycqa.org/en/latest/user_guide/usage/run.html#exit-codes

* pylint: use exit code 32 when invalid arguments are passed

* pylint: add failing test when ambiguous abbreviated parameters are set in a config file

This is confusing behaviour. The output is:

```
usage: pylint [options]
pylint: error: ambiguous option: --no could match --notes, --notes-rgx, --no-docstring-rgx
```

The exit code is 2 which doesn't match the documentation: https://pylint.pycqa.org/en/latest/user_guide/usage/run.html#exit-codes

* pylint: use exit code 32 when ambiguous abbreviated parameters are set in a config file

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
(cherry picked from commit 62232b3)
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 244e1cf

Pierre-Sassoulas pushed a commit that referenced this pull request Dec 15, 2022
…rror code (#7931) (#7943)

Returning 2 here is confusing as it doesn't match the documentation: https://pylint.pycqa.org/en/latest/user_guide/usage/run.html#exit-codes

* pylint: use exit code 32 when invalid arguments are passed

* pylint: add failing test when ambiguous abbreviated parameters are set in a config file

This is confusing behaviour. The output is:

```
usage: pylint [options]
pylint: error: ambiguous option: --no could match --notes, --notes-rgx, --no-docstring-rgx
```

The exit code is 2 which doesn't match the documentation: https://pylint.pycqa.org/en/latest/user_guide/usage/run.html#exit-codes

* pylint: use exit code 32 when ambiguous abbreviated parameters are set in a config file

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
(cherry picked from commit 62232b3)

Co-authored-by: David Lawson <dmrlawson@gmail.com>
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.

None yet

4 participants