Skip to content

Fixes duplicate-code check with ignore-imports#9147

Merged
jacobtylerwalls merged 9 commits into
pylint-dev:mainfrom
theirix:duplicate-code-conditional-imports
Oct 22, 2023
Merged

Fixes duplicate-code check with ignore-imports#9147
jacobtylerwalls merged 9 commits into
pylint-dev:mainfrom
theirix:duplicate-code-conditional-imports

Conversation

@theirix
Copy link
Copy Markdown
Contributor

@theirix theirix commented Oct 13, 2023

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Description

Improves detection of imports under conditionals. Limited checks by constant conditions such as True/typing.TYPE_CHECKING.

Closes #8914

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 13, 2023

Codecov Report

Merging #9147 (c5cbe23) into main (0796dfa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9147   +/-   ##
=======================================
  Coverage   95.76%   95.76%           
=======================================
  Files         173      173           
  Lines       18680    18681    +1     
=======================================
+ Hits        17889    17890    +1     
  Misses        791      791           
Files Coverage Δ
pylint/checkers/similar.py 96.29% <100.00%> (+<0.01%) ⬆️

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

Inference seems like overkill here, since we just need to traverse the ast. Something like this passes the unit test you've contributed. It causes a failure in test_no_hide_code_with_imports, but that puzzles me as that expected result looks incorrect (we should remove the --ignore-imports flag from that case, no?

Comment thread pylint/checkers/similar.py Outdated
@jacobtylerwalls jacobtylerwalls added this to the 3.0.2 milestone Oct 14, 2023
Copy link
Copy Markdown
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for the updates; very helpful. Requesting one last test case to pass and then this should be ready to merge!

Comment thread pylint/checkers/similar.py Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

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

This comment was generated for commit c5cbe23

Copy link
Copy Markdown
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@Pierre-Sassoulas Pierre-Sassoulas added Bug 🪲 duplicate-code Related to code duplication checker labels Oct 16, 2023
@theirix
Copy link
Copy Markdown
Contributor Author

theirix commented Oct 16, 2023

Thanks for the contribution!

Sure, thank you for the extensive review and suggestions.

Copy link
Copy Markdown
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.

We're not checking duplicate-code in the primers, might be beneficial to check locally or exceptionally/temporarily enable it in the CI in this case ? (wdyt @jacobtylerwalls ?)

@jacobtylerwalls
Copy link
Copy Markdown
Member

Yeah, that's worth a try.

@jacobtylerwalls
Copy link
Copy Markdown
Member

I ran the primer locally, minus pandas and home-assistant. Generated a lot of noisy, trivial output. Too much to paste here. Next time I'll do this on CI on a fork so we get a more readable output.

I did see a handful of wanted changes like this:

No longer emitted: (astroid)
{'type': 'refactor', 'module': 'astroid.brain.brain_hashlib', 'obj': '', 'line': 1, 'column': 0, 'endLine': None, 'endColumn': None, 'path': 'tests/.pylint_primer_tests/pylint-dev/astroid/astroid/brain/brain_hashlib.py', 'symbol': 'duplicate-code', 'message': 'Similar lines in 2 files\n==astroid.nodes.node_classes:[56:63]\n==astroid.nodes.node_ng:[40:51]\nif sys.version_info >= (3, 11):\n from typing import Self\nelse:\n from typing_extensions import Self\n\nif TYPE_CHECKING:\n from astroid import nodes', 'message-id': 'R0801'}

@jacobtylerwalls jacobtylerwalls added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/4.0.x labels Oct 21, 2023
@jacobtylerwalls
Copy link
Copy Markdown
Member

(Just clarifying for folks who might not be aware of the flakiness in the primer checks: the "noisy, trivial output" I referred to is probably from some unrelated mutability of cached objects in astroid, not from anything wrong in this PR.)

@jacobtylerwalls jacobtylerwalls merged commit 67f20bd into pylint-dev:main Oct 22, 2023
github-actions Bot pushed a commit that referenced this pull request Oct 22, 2023
Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
(cherry picked from commit 67f20bd)
jacobtylerwalls pushed a commit that referenced this pull request Oct 22, 2023
(cherry picked from commit 67f20bd)

Co-authored-by: theirix <theirix@gmail.com>
@Pierre-Sassoulas
Copy link
Copy Markdown
Member

Thank you for checking Jacob !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backported Bug 🪲 duplicate-code Related to code duplication checker False Positive 🦟 A message is emitted but nothing is wrong with the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate Code warning occurs for conditional imports even with ignore-imports=yes

3 participants