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

Define Protocol as abstract to prevent abstract-method FP #7839

Merged
merged 4 commits into from Nov 30, 2022

Conversation

clavedeluna
Copy link
Collaborator

@clavedeluna clavedeluna commented Nov 24, 2022

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Closes #7209

I'm not sure if I just got lucky in this very short fix or I'm completely wrong here in adding a Protocol check to the is_abstract utility method....

@clavedeluna clavedeluna changed the title make protocols be defined as abstract Define Protocol as abstrac to prevent abstract-method FP Nov 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Blocked 🚧 Blocked by a particular issue False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/2.15.x labels Nov 24, 2022
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Nov 24, 2022
@clavedeluna clavedeluna marked this pull request as ready for review November 24, 2022 20:05
@clavedeluna
Copy link
Collaborator Author

I see I need to add an .rc file so this only applies to py3.8+, but I'll wait to hear from maintainers about if this approach is even the right thing to do.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Turns out I never submitted my initial review πŸ˜“

Comment on lines 1 to 6
"""Test that classes inheriting from protocols should not warn about abstract-method."""
# pylint: disable=too-few-public-methods,disallowed-name,invalid-name
from abc import abstractmethod
from typing import Protocol, Literal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"""Test that classes inheriting from protocols should not warn about abstract-method."""
# pylint: disable=too-few-public-methods,disallowed-name,invalid-name
from abc import abstractmethod
from typing import Protocol, Literal
"""Test that classes inheriting from protocols should not warn about abstract-method."""
# pylint: disable=too-few-public-methods,disallowed-name,invalid-name
from abc import abstractmethod
from typing import Protocol, Literal

@coveralls
Copy link

coveralls commented Nov 25, 2022

Pull Request Test Coverage Report for Build 3568918104

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.445%

Totals Coverage Status
Change from base Build 3559949152: 0.01%
Covered Lines: 17603
Relevant Lines: 18443

πŸ’› - Coveralls

@github-actions

This comment has been minimized.

@clavedeluna clavedeluna added Needs review πŸ” Needs to be reviewed by one or multiple more persons and removed Needs review πŸ” Needs to be reviewed by one or multiple more persons labels Nov 25, 2022
@jacobtylerwalls jacobtylerwalls changed the title Define Protocol as abstrac to prevent abstract-method FP Define Protocol as abstract to prevent abstract-method FP Nov 26, 2022
return True
except astroid.InferenceError:
continue
return False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out is_protocol_class was incorrectly defined since it concluded that if any parent/grandparent class inherited from Protocol, this class was one too, but as we discussed the class has to inherit from Protocol itself. This definition was already in the code base so I just moved it here

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

@DanielNoord DanielNoord merged commit 85e7d93 into pylint-dev:main Nov 30, 2022
github-actions bot pushed a commit that referenced this pull request Nov 30, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Dec 4, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.9 milestone Dec 4, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Dec 5, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.15.9, 2.15.8 Dec 5, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Dec 5, 2022
)

(cherry picked from commit 85e7d93)

Co-authored-by: Dani Alcala <112832187+clavedeluna@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.

false positive: abstract-method for typing.Protocol inheritance with abc.abstractmethod
4 participants