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.

Sign up for GitHub

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

Jump to bottom

Only emit bad-reversed-sequence on dictionaries if below py3.8 (#3940) #3958

Merged
merged 2 commits into from Nov 30, 2020
Merged

Only emit bad-reversed-sequence on dictionaries if below py3.8 (#3940) #3958

merged 2 commits into from Nov 30, 2020

Conversation

ethan-leba
Copy link
Contributor

Description

Only emit bad-reversed-sequence on dictionaries if below py3.8

Type of Changes

Type
✓ 🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

Closes #3940

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.82% when pulling 13867ec on ethan-leba:eleba/reversed38 into 0847265 on PyCQA:master.

@coveralls
Copy link

coveralls commented Nov 27, 2020 •
edited

Coverage Status

Coverage decreased (-0.002%) to 90.857% when pulling bf1d1e0 on ethan-leba:eleba/reversed38 into b1f2204 on PyCQA:master.

@ethan-leba ethan-leba marked this pull request as ready for review November 28, 2020 15:57
@Pierre-Sassoulas
Copy link
Member

Thanks for the merge request @ethan-leba, very clean ! I saw in the linked issue that the original poster said it's fine for "Python 3.7 and up", so shouldn't it be only below 3.7 instead of 3.8 ?

@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Nov 29, 2020
@ethan-leba
Copy link
Contributor Author

Thanks! I think the poster of the original issue must've been mistaken, as it appears this change was introduced in Py3.8 (see 5th bullet point here)

@Pierre-Sassoulas
Copy link
Member

Thanks you for the doc, you're right :) Also, I wonder if everything in the test case before line 62 could be in a functional test used for all version of python ? Isn't the dict test the only one that is specific to python 3.8 and above ?

@ethan-leba
Remove hardcoded error for bad-reversed-sequence on dicts
22e805e 
As a consequence, pylint will only emit `bad-reversed-sequence` on
dictionaries if below py3.8, as a `__reversed__` dunder was added to
dicts on py3.8
ethan-leba
ethan-leba commented Nov 29, 2020
View reviewed changes
Copy link
Contributor Author

@ethan-leba ethan-leba left a comment

Choose a reason for hiding this comment

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

Addressed the redundancies in the functional tests, also decided to take a slightly different approach to this fix

pylint/checkers/base.py
Comment on lines -1527 to 1532
if argument._proxied.name == "dict" and utils.is_builtin_object(
argument._proxied
):
self.add_message("bad-reversed-sequence", node=node)
return
if any(
ancestor.name == "dict" and utils.is_builtin_object(ancestor)
for ancestor in argument._proxied.ancestors()
for ancestor in itertools.chain(
(argument._proxied,), argument._proxied.ancestors()
)
):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of a check for the system version, I removed the hardcoded add_message on dicts. We should be able to find the __reversed__ dunder on py3.8 but not >py3.7, which i think should address this bug in a cleaner way IMO. I think the performance should be the same, but LMK if this seems too slow, or if there's a reason why this might not work!

Pierre-Sassoulas
Pierre-Sassoulas requested changes Nov 29, 2020
View reviewed changes
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.

Wow, that MR that took a 360 ..for the better :D Actually one minor comment, you do not need the empty bad_reversed_sequence_py38.txt anymore, thanks to a fresh commit from this afternoon ;)

@ethan-leba
Copy link
Contributor Author

@Pierre-Sassoulas Deleted the empty text file, LMK if there's any other changes that need to be made 👍

@Pierre-Sassoulas Pierre-Sassoulas merged commit 6e1cca1 into pylint-dev:master Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

false positive bad-reversed-sequence when used with dicts
3 participants
@ethan-leba @coveralls @Pierre-Sassoulas