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 empty no_docstring_rgx not working correctly #5084

Closed
wants to merge 2 commits into from

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
🐛 Bug fix
🔨 Refactoring

Description

Turns out we did not handle empty regex options correctly. The referenced issue was broken because whenever the no_docstring_rgx option was empty it would try to match re.compile("") which always matched and would thus never check a parameter.

The change to get_global_option() is necessary for it to actually return a Pattern[str] for those options. A problem I couldn't solve was that it is impossible (I think) to tell mypy that the option belongs to GLOBAL_OPTION_PATTERN and should therefore return a Pattern[str]. This is because you can't use isinstance on Literal and there is no good solution to do this any other way (using cast also doesn't work).

/CC @cdce8p Because you helped with #4978 and might have a solution for the mypy issue that I don't know about 😄

Closes #4136

@DanielNoord DanielNoord added Bug 🪲 False Negative 🦋 No message is emitted but something is wrong with the code Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Sep 28, 2021
@DanielNoord DanielNoord changed the title No docstring rgx Fix issue with regex options and no_docstring_rgx Sep 28, 2021
@coveralls
Copy link

coveralls commented Sep 28, 2021

Pull Request Test Coverage Report for Build 1281877563

  • 14 of 16 (87.5%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 93.094%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/utils/utils.py 13 15 86.67%
Totals Coverage Status
Change from base Build 1281362551: -0.004%
Covered Lines: 13372
Relevant Lines: 14364

💛 - Coveralls

@DanielNoord
Copy link
Collaborator Author

❯ pip install -e .
Obtaining file:///Users/daniel/DocumentenLaptop/Programming/Github/pylint
Requirement already satisfied: platformdirs>=2.2.0 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from pylint==2.11.2.dev0) (2.2.0)
Requirement already satisfied: astroid<2.9,>=2.8.0 in /Users/daniel/DocumentenLaptop/Programming/Github/astroid (from pylint==2.11.2.dev0) (2.8.0)
Requirement already satisfied: isort<6,>=4.2.5 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from pylint==2.11.2.dev0) (5.9.3)
Requirement already satisfied: mccabe<0.7,>=0.6 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from pylint==2.11.2.dev0) (0.6.1)
Requirement already satisfied: toml>=0.9.2 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from pylint==2.11.2.dev0) (0.10.2)
Requirement already satisfied: typing-extensions>=3.10.0 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from pylint==2.11.2.dev0) (3.10.0.0)
Requirement already satisfied: lazy_object_proxy>=1.4.0 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from astroid<2.9,>=2.8.0->pylint==2.11.2.dev0) (1.6.0)
Requirement already satisfied: wrapt<1.13,>=1.11 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from astroid<2.9,>=2.8.0->pylint==2.11.2.dev0) (1.12.1)
Requirement already satisfied: setuptools>=20.0 in /Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/lib/python3.9/site-packages (from astroid<2.9,>=2.8.0->pylint==2.11.2.dev0) (56.0.0)
Installing collected packages: pylint
  Attempting uninstall: pylint
    Found existing installation: pylint 2.11.2.dev0
    Uninstalling pylint-2.11.2.dev0:
      Successfully uninstalled pylint-2.11.2.dev0
  Running setup.py develop for pylint
Successfully installed pylint-2.11.2.dev0
WARNING: You are using pip version 21.2.2; however, version 21.2.4 is available.
You should consider upgrading via the '/Users/daniel/.pyenv/versions/3.9.6/envs/pylint-3.9.6/bin/python3.9 -m pip install --upgrade pip' command.
❯ pylint '/Users/daniel/DocumentenLaptop/Programming/Github/pylint/pylint/utils/utils.py'

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Not sure what pre-commit is failing on. Passes locally when installing pylint from working directory.

pylint/utils/utils.py Show resolved Hide resolved
@DanielNoord DanielNoord changed the title Fix issue with regex options and no_docstring_rgx Fix empty no_docstring_rgx not working correctly Sep 29, 2021
@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Sep 29, 2021

I wonder why the default value for no_docstring_rgx was ^_.
Perhaps there was a logic behind it, but users such as the one who initially reported this bug will never figure out that they need to add no_docstring_rgx= to their pylintrc for it to check all functions. I think changing it to be empty is more in line with what users will expect the default value of a regex pattern to be.

I would like confirmation on changing this default value before adding a commit that fixes the tests that fail because of the change.

Furthermore, it seems like the unittests are really prone to incorrectly testing settings. The change to "no_docstring_rgx": re.compile(""), in tests/extensions/test_check_docs.py shows that we might set settings to certain values with incorrect types and therefore passing/failing tests incorrectly. This is not something we can fix immediately but is something to keep in mind!

Edit: I can't reopen this because of force-pushing the branch. Opening a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 False Negative 🦋 No message is emitted but something is wrong with the code Needs review 🔍 Needs to be reviewed by one or multiple more persons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__init__ arguments are not checked correctly by docparams
3 participants