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

Fixes duplicate-code check with ignore-imports #9147

Merged

Conversation

theirix
Copy link
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

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
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?

pylint/checkers/similar.py Outdated Show resolved Hide resolved
@jacobtylerwalls jacobtylerwalls added this to the 3.0.2 milestone Oct 14, 2023
Copy link
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!

pylint/checkers/similar.py Outdated Show resolved Hide resolved
@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
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
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
Contributor Author

theirix commented Oct 16, 2023

Thanks for the contribution!

Sure, thank you for the extensive review and suggestions.

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.

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
Member

Yeah, that's worth a try.

@jacobtylerwalls
Copy link
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/3.2.x labels Oct 21, 2023
@jacobtylerwalls
Copy link
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
52 checks passed
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
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