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

Refactor and add typing to NodeNG.frame() #1225

Merged
merged 5 commits into from
Oct 25, 2021
Merged

Refactor and add typing to NodeNG.frame() #1225

merged 5 commits into from
Oct 25, 2021

Conversation

DanielNoord
Copy link
Collaborator

Steps

  • Write a good description on what the PR does.

Description

/CC @cdce8p

As discussed in #1224

I also added Lambda and added it to the docstrings as we seemed to have forgotten to add it.

Type of Changes

Type
✓ 🔨 Refactoring

Related Issue

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Oct 24, 2021
@DanielNoord DanielNoord added this to the 2.8.4 milestone Oct 24, 2021
@DanielNoord
Copy link
Collaborator Author

At some point I will learn to skip for 3.8... 😅

@Pierre-Sassoulas
Copy link
Member

At some point I will learn to skip for 3.8...

Well, no pressure : it bring only temporary benefit, after the 27th of June 2023 it won't matter anymore 😄

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Oct 25, 2021
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.

I like the new added tests 👍

astroid/nodes/scoped_nodes.py
@@ -3054,3 +3062,13 @@ def _get_assign_nodes(self):
child_node._get_assign_nodes() for child_node in self.body
)
return list(itertools.chain.from_iterable(children_assign_nodes))

def frame(self: T) -> T:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use ClassDef here if we duplicate the function for typing purpose ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could, but this will tell the typing engine the same. self gets set to T so T is ClassDef here. This works similar to scope() which also is able to distinguish between NodeNG.scope() and ClassDef.scope(). You can test the latter in pylint yourself already!

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation :)

@DanielNoord DanielNoord marked this pull request as ready for review October 25, 2021 12:17
@Pierre-Sassoulas Pierre-Sassoulas merged commit 0a43490 into pylint-dev:main Oct 25, 2021
@DanielNoord DanielNoord requested a review from cdce8p October 25, 2021 22:13
@DanielNoord
Copy link
Collaborator Author

@cdce8p Sorry for requesting a review after this has been merged. I know you wanted to review this change so I thought I would tag you either way.

@cdce8p
Copy link
Member

cdce8p commented Oct 25, 2021

@cdce8p Sorry for requesting a review after this has been merged. I know you wanted to review this change so I thought I would tag you either way.

I had already started with the review when Pierre merged it. Wasn't fast enough I guess ;)

Nothing major though. The tests could have been written slightly differently to emphasize the difference between scope and frame. Not worth changing now.

Aside from that I was a bit surprised that Lambda has it's own frame. I tried to remove it just to see what would happen and got multiple errors. For better or worse it seems like we are stuck with the way things are. Just a bit curious what the actual difference between scope and frame is supposed to be then. Comprehensions are the only ones where those aren't the same.

@cdce8p cdce8p removed their request for review October 25, 2021 22:20
@DanielNoord DanielNoord deleted the frame branch October 25, 2021 22:25
@DanielNoord
Copy link
Collaborator Author

Aside from that I was a bit surprised that Lambda has it's own frame. I tried to remove it just to see what would happen and got multiple errors. For better or worse it seems like we are stuck with the way things are. Just a bit curious what the actual difference between scope and frame is supposed to be then. Comprehensions are the only ones where those aren't the same.

I don't really know myself as well 😅 I know I tried googling for "python frame" and didn't find a direct definition as well. Might it be something introduced in this project some years ago of which the definition has now been lost?

@Pierre-Sassoulas
Copy link
Member

Sorry for merging before your review @cdce8p ! I'm trying to wrap up pylint 2.12 because there are a lot of bug fixes in it and some fixed issues are getting duplicates (also pylint is broken for < 3.6.2 users)

@DanielNoord DanielNoord mentioned this pull request Nov 20, 2021
Merged
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
Maintenance Discussion or action around maintaining astroid or the dev workflow
Projects
None yet
Milestone
2.8.4
Development

Successfully merging this pull request may close these issues.
3 participants
@DanielNoord @Pierre-Sassoulas @cdce8p