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 FunctionDef isinstance checks #8607

Merged
merged 2 commits into from
Apr 23, 2023
Merged

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Apr 23, 2023

Description

Fallout from pylint-dev/astroid#2115 - required for the next astroid update.
Fix isinstance checks now that nodes.FunctionDef doesn't inherit from nodes.Lambda.

Changes are compatible with the current astroid version, so don't need to wait to fix these.

@cdce8p cdce8p added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry labels Apr 23, 2023
@cdce8p cdce8p requested a review from DanielNoord April 23, 2023 14:47
@codecov
Copy link

codecov bot commented Apr 23, 2023

Codecov Report

Merging #8607 (3bc915a) into main (3d036b7) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8607   +/-   ##
=======================================
  Coverage   95.92%   95.92%           
=======================================
  Files         174      174           
  Lines       18415    18415           
=======================================
  Hits        17664    17664           
  Misses        751      751           
Impacted Files Coverage Δ
pylint/checkers/typecheck.py 96.46% <100.00%> (ø)
pylint/checkers/utils.py 96.05% <100.00%> (ø)
pylint/checkers/variables.py 97.25% <100.00%> (ø)

@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! Just so all reviewers are aware, we will still need another round of cleanup after bumping to astroid 2.16, when we could remove things like this last line:

if (
isinstance(scope, nodes.Lambda)
and not isinstance(scope, nodes.FunctionDef)

Either then or now, we could tidy up some comments like this:

There's also a comment to clean up in `_determine_callable(), but we can do that later:

# Ordering is important, since BoundMethod is a subclass of UnboundMethod,
# and Function inherits Lambda.

pylint/checkers/variables.py Outdated Show resolved Hide resolved
pylint/checkers/utils.py Outdated Show resolved Hide resolved
@cdce8p cdce8p requested a review from DudeNr33 as a code owner April 23, 2023 16:34
@cdce8p cdce8p removed the request for review from DudeNr33 April 23, 2023 16:43
@DanielNoord
Copy link
Collaborator

Yeah, I think this change is very likely to cause a need to do a 3.0 and perhaps 4.0 quickly after it. Then again, still think it is a good change.

Thanks @cdce8p and @jacobtylerwalls for taking these first steps towards compatibility!

@jacobtylerwalls
Copy link
Member

Yeah, I think this change is very likely to cause a need to do a 3.0 and perhaps 4.0 quickly after it. Then again, still think it is a good change.

Agree about 3.0. Let's slow the release cadence down for a little bit, and release both 3.0 in pylint and astroid when we're ready for python 3.12. We may not need a 4.0 then.

@DanielNoord
Copy link
Collaborator

Yeah, I think this change is very likely to cause a need to do a 3.0 and perhaps 4.0 quickly after it. Then again, still think it is a good change.

Agree about 3.0. Let's slow the release cadence down for a little bit, and release both 3.0 in pylint and astroid when we're ready for python 3.12. We may not need a 4.0 then.

Also fine with me, although we should definitely release 3.0a1 soon. I'd like to see behaviour in pylint as I'm not all that sure about the changes to some of the constructors. I think we're making definitive improvements but might have been too strict in some places.

@Pierre-Sassoulas
Copy link
Member

+1, we can keep the release cadence by releasing alpha/betas and still wait to support python 3.12 for pylint 3.0. There's a lot to do in #3512 still, we'll probably release pylint 3.0 after python 3.12 is out in any case.

@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 3bc915a

@cdce8p cdce8p merged commit 7826795 into pylint-dev:main Apr 23, 2023
@cdce8p cdce8p deleted the fix-FunctionDef branch April 23, 2023 20:45
@cdce8p cdce8p added the Astroid Related to astroid label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Astroid Related to astroid Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants