-
Notifications
You must be signed in to change notification settings - Fork 0
Argument Clinic: Remove METHOD_INIT and METHOD_NEW function kinds #23
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| @functools.cached_property | ||
| def new_or_init(self) -> bool: | ||
| return self.name in ("__new__", "__init__") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be a cached_property anymore; we can just assign it eagerly in __post_init__:
| @functools.cached_property | |
| def new_or_init(self) -> bool: | |
| return self.name in ("__new__", "__init__") | |
| self.new_or_init = self.name in ("__new__", "__init__") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's fine. But what do you think of this refactor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I did not mean to be dismissive! Thanks for the suggestion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you're right; the overall concept is more important!
I think it looks good. Conceptually, I think these aren't really distinct categories of methods; they're subcategories of (respectively) CALLABLE and CLASS_METHOD. So we're simplifying some code while also making it conceptually cleaner, which is nice.
The benefit doesn't seem huge to me right now, but I'll trust you when you say that this clears the way for further refactorings :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to formulate my motivation for doing this. I just need to find time to jot it down :)
| ) -> str | None: | ||
| type, _ = correct_name_for_self(f) | ||
| if f.kind in (METHOD_INIT, METHOD_NEW, STATIC_METHOD, CLASS_METHOD): | ||
| if f.kind in (STATIC_METHOD, CLASS_METHOD) or f.name == "__init__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if f.kind in (STATIC_METHOD, CLASS_METHOD) or f.name == "__init__": | |
| if f.kind in (STATIC_METHOD, CLASS_METHOD) or f.new_or_init: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reads slightly nicer.
| f: Function | ||
| ) -> tuple[str, str]: | ||
| if f.kind in (CALLABLE, METHOD_INIT): | ||
| if f.kind is CALLABLE or f.name == "__init__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if f.kind is CALLABLE or f.name == "__init__": | |
| if f.kind is CALLABLE: |
|
I think you should assign three attributes in class Function:
# <-- snip -->
def __post_init__(self) -> None:
# <-- snip -->
self.init_method = self.name == "__init__"
self.new_method = self.name == "__new__"
self.new_or_init = self.init_method or self.new_methodThen you wouldn't have to have so many "magic strings" everywhere -- everywhere you're currently doing |
Yeah, that's neat. I thought of doing something like that; I'm not a fan of the |
No description provided.