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

Add typing to the parent attribute of ExceptHandler #1385

Closed
wants to merge 1 commit into from

Conversation

DanielNoord
Copy link
Collaborator

Steps

  • Write a good description on what the PR does.

Description

Inspired by the work of @jacobtylerwalls in pylint-dev/pylint#5764.

Is there a good way to make parent non-optional?

Type of Changes

Type
🔨 Refactoring

Related Issue

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Feb 8, 2022
@DanielNoord DanielNoord added this to the 2.10.0 milestone Feb 8, 2022
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.

Very nice !

@cdce8p cdce8p self-requested a review February 8, 2022 22:37
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

I recommend not to move forward with this exact PR at this time.

It would be better to implement the parent decorator first (if it works as expected). Furthermore, I'm working on some extensive rebuilder changes. It's probably better to consider it after those are done. There is also the rewrite for the Try nodes I mentioned in #1384 (review). Lastly, assuming the parent should be a TryExcept node might make sense here. That doesn't mean though that similar cases also do. In general we should be cautious about such assumptions, not all astroid plugins might adhere to the official Python AST. Adding an additional isinstance check in pylint certainly doesn't hurt.

def __init__(
self,
lineno: Optional[int] = None,
col_offset: Optional[int] = None,
parent: Optional[NodeNG] = None,
# TODO: Make parent non-optional
Copy link
Member

Choose a reason for hiding this comment

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

This wouldn't really work for plugins / brain modules. They might want to construct a Node outside the tree first before inserting it in the end.

Comment on lines +2526 to +2527
# TODO: Make parent non-optional
parent: Optional["nodes.TryExcept"]
Copy link
Member

Choose a reason for hiding this comment

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

Saying the parent attribute is required (except for Module nodes), might work with a decorator.

    def __init__(..., parent: Optional["NodeNG"], ...):
        self._parent = parent  # this needs to stay optional

    @property
    def parent(self) -> "NodeNG":
        assert self._parent  # here, we make sure it has actually been assigned
        return self._parent

    @parent.setter
    def parent(self, value: "NodeNG") -> None:
        self._parent = value

If you would like to explore that further in a new PR, an additional tip. Since Module inherits from NodeNG and doesn't have a parent, the decorator can't be added there. You'd probably need to create a new subclass which all nodes (except for Module) inherit from to implement it.

@DanielNoord
Copy link
Collaborator Author

Good points @cdce8p! I'll wait on the rebuilder refactor before doing anything like this. Let me know if I can be of any help anywhere!

@DanielNoord DanielNoord closed this Feb 8, 2022
@DanielNoord DanielNoord deleted the except-parent branch February 8, 2022 23:26
@DanielNoord DanielNoord removed this from the 2.10.0 milestone Feb 8, 2022
@cdce8p
Copy link
Member

cdce8p commented Feb 8, 2022

Good points @cdce8p! I'll wait on the rebuilder refactor before doing anything like this. Let me know if I can be of any help anywhere!

I'll probably need to write some kind of design doc (either in the PR or in a separate issue). Will make sure to let you know when I do, feedback should definitely help!

The parent decorator concept shouldn't interfere with the refactor if you want to look into that.

@cdce8p
Copy link
Member

cdce8p commented Feb 8, 2022

The parent decorator concept shouldn't interfere with the refactor if you want to look into that.

It's also something I hope to use for attributes that are currently defined as optional but never are. Like the name and attrname attributes on AssignName, DelName, Name, AssignAttr, and DelAttr.
If you have some time, implementing the decorator concept for those would probably be quite useful.

@DanielNoord
Copy link
Collaborator Author

I can certainly take a look at the decorator concept when I get some time. Probably after pylint 2.13 and astroid 2.10 though 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining astroid or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants