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 unnecessary-comprehension provides incorrect suggestion #9172

Merged
merged 6 commits into from
Nov 17, 2023

Conversation

C0DE-SLAYER
Copy link
Contributor

@C0DE-SLAYER C0DE-SLAYER commented Oct 21, 2023

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Closes #9109

@jacobtylerwalls
Copy link
Member

Thanks for the patch!

Based on the test results, you'll notice you'll need to adjust the logic of #4500 to make it a little dumber (and therefore more like the original intent of #2999).

@github-actions

This comment has been minimized.

@C0DE-SLAYER
Copy link
Contributor Author

@jacobtylerwalls i am a beginner and having a hard time solving can you help me what i have to do to solve the issue. it would be a great help! thanks

@Pierre-Sassoulas
Copy link
Member

I'm on mobile si I can't link the specific code but the message is raised in the add_message function, the message is dynamic and the argument given to add_message used in %s later is what need to be modified.

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #9172 (1bc8f5e) into main (df0800e) will increase coverage by 0.02%.
Report is 22 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9172      +/-   ##
==========================================
+ Coverage   95.76%   95.79%   +0.02%     
==========================================
  Files         173      173              
  Lines       18692    18722      +30     
==========================================
+ Hits        17901    17935      +34     
+ Misses        791      787       -4     
Files Coverage Ξ”
pylint/checkers/refactoring/refactoring_checker.py 98.24% <100.00%> (+<0.01%) ⬆️

... and 11 files with indirect coverage changes

@github-actions

This comment has been minimized.

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.

Thank you for your contribution, it looks promising, do you mind adding the functional tests from #8577

a = [[1, 2], [3, 4]]
b = [(x, y) for x, y in a]  # flagged as unnecessary-comprehension

And #9109:

#pylint: disable=missing-module-docstring
a = [1, 2, 3]
b = [x for x in a]
b[0] = 0
print(a) # [1, 2, 3]

in tests/functional/u/unnecessary/unnecessary_comprehension.py, please ?

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/3.2.x labels Oct 27, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.3 milestone Oct 27, 2023
@Pierre-Sassoulas
Copy link
Member

Also a bug / or false positive changelog fragment would be nice for this one.

@C0DE-SLAYER
Copy link
Contributor Author

Thank you for your contribution, it looks promising, do you mind adding the functional tests from #8577

a = [[1, 2], [3, 4]]
b = [(x, y) for x, y in a]  # flagged as unnecessary-comprehension

And #9109:

#pylint: disable=missing-module-docstring
a = [1, 2, 3]
b = [x for x in a]
b[0] = 0
print(a) # [1, 2, 3]

in tests/functional/u/unnecessary/unnecessary_comprehension.py, please ?

in the unnecessary_comprehension.py on line 5 it can be use for 9109 and
for 8577 ' [(x, y) for x, y in iterable] # [unnecessary-comprehension] ' is this good

@C0DE-SLAYER
Copy link
Contributor Author

C0DE-SLAYER commented Oct 28, 2023

Also a bug / or false positive changelog fragment would be nice for this one.

where to do that in the doc/whatsnew/fragment or any where else and what it would be helpfull thanks!

@jacobtylerwalls
Copy link
Member

where to do that in the doc/whatsnew/fragment or any where else and what it would be helpfull thanks!

Check out the contributor docs for some info on documenting your pull request.

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 news entry! Spotted one typo.

doc/whatsnew/fragments/9172.false_positive Outdated Show resolved Hide resolved
doc/whatsnew/fragments/9172.false_positive Outdated Show resolved Hide resolved
@jacobtylerwalls jacobtylerwalls enabled auto-merge (squash) November 15, 2023 19:54
@jacobtylerwalls
Copy link
Member

Thanks for the contribution! 🎁

Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on sentry:
The following messages are now emitted:

  1. unnecessary-comprehension:
    Unnecessary use of a comprehension, use list(self.get_targets()) instead.
    https://github.com/getsentry/sentry/blob/94245776a36896510af2e305a4b121e00a1669aa/src/sentry/incidents/action_handlers.py#L185
  2. unnecessary-comprehension:
    Unnecessary use of a comprehension, use list(self.unregistered_options) instead.
    https://github.com/getsentry/sentry/blob/94245776a36896510af2e305a4b121e00a1669aa/src/sentry/runner/commands/presenters/slackpresenter.py#L95

The following messages are no longer emitted:

  1. unnecessary-comprehension:
    Unnecessary use of a comprehension, use self.get_targets() instead.
    https://github.com/getsentry/sentry/blob/94245776a36896510af2e305a4b121e00a1669aa/src/sentry/incidents/action_handlers.py#L185
  2. unnecessary-comprehension:
    Unnecessary use of a comprehension, use self.unregistered_options instead.
    https://github.com/getsentry/sentry/blob/94245776a36896510af2e305a4b121e00a1669aa/src/sentry/runner/commands/presenters/slackpresenter.py#L95

This comment was generated for commit 1bc8f5e

@jacobtylerwalls jacobtylerwalls dismissed Pierre-Sassoulas’s stale review November 17, 2023 12:26

requested test coverage added

@jacobtylerwalls jacobtylerwalls merged commit 6f83d5a into pylint-dev:main Nov 17, 2023
44 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 17, 2023
jacobtylerwalls pushed a commit that referenced this pull request Nov 17, 2023
(cherry picked from commit 6f83d5a)

Co-authored-by: Sayyed Faisal Ali <80758388+C0DE-SLAYER@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported 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.

unnecessary-comprehension provides incorrect suggestion
3 participants