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

Revert #893: Avoid setting a Call as a base for classes from six.with_metaclass() #1622

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Jun 15, 2022

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

#893 intended to fix a false positive for unused-import of six in a pattern like class Thing(six.with_metaclass(meta, ancestor)) by making the nodes.Call containing six.with_metaclass into a base. Then, this violated assumptions elsewhere, leading to the crash in pylint-dev/pylint#5935.

#893 was itself a partial reversion of #841, a fix for #713. #713 is essentially the same issue as pylint-dev/pylint#5935. So what I'm suggesting is that we revert to #841, and then find another way to deal with the unused-import in pylint, if at all.

(I'd much rather have a false positive than a crash. And the class of false positives is larger than anything to do with six.with_metaclass anyway: see pylint-dev/pylint#1630. So we shouldn't treat six as more special.)

Let's not backport this, to give us time to make a decision about what to do in pylint with pylint-dev/pylint#1630.

Type of Changes

Type
🐛 Bug fix

Related Issue

Refs pylint-dev/pylint#5935

@jacobtylerwalls jacobtylerwalls added this to the 2.12.0 milestone Jun 15, 2022
@jacobtylerwalls jacobtylerwalls changed the title Revert #893: Avoid setting a Call as a base for metaclasses from six.with_metaclass() Revert #893: Avoid setting a Call as a base for classes from six.with_metaclass() Jun 15, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2503836953

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0008%) to 92.24%

Totals Coverage Status
Change from base Build 2493049607: 0.0008%
Covered Lines: 9367
Relevant Lines: 10155

💛 - Coveralls

@hippo91
Copy link
Contributor

hippo91 commented Jun 23, 2022

@jacobtylerwalls @Pierre-Sassoulas i'm ok with this PR.
It is indeed much preferable to have a false positive than a crash.
Sorry for #893. A bad answer to the initial problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants