-
Notifications
You must be signed in to change notification settings - Fork 80
docstring: Use update_wrapper to propagate the types #4295
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
| raise ValueError("origin class has no %s method" % dest.__name__) | ||
|
|
||
| dest.__doc__ = origin.__doc__ | ||
| functools.update_wrapper(dest, origin) |
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.
Do we want to specify the assigned and updated. According to the name of the function I would expect to have only __doc__ ?
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.
To me this function is not properly named.
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.
Indeed I think this is related to the questions of thomas.
- If the behavior change -> the function might be renamed
- then if the behavior change the question is: is it safe for the existing source code ? This function is also exposed publicly so changing the behavior on other source code might be an issue.
|
That sounds risky to propagate the typing in If copying typing is needed, a different decorator would be best IMO. |
I don't understand what you mean. To me, we should have an So, in fact i would be in favor of renaming this helper. Even if we only support classname as argument (which is safer in this case). |
|
You can achieve the same with class A:
def f(self, a: int, b: str = "") -> str:
"""docstring"""
return f"{a}-{b}"
class B(A):
@functools.wraps(A.f)
def f(self, a, b):
return "B"I would keep |
Propagated the types when
@docstringis used.See update_wrapper: https://docs.python.org/3/library/functools.html#functools.update_wrapper
The downfall is that we probably can't override the types, but that's probably not a common usecase.
As the documentation is on it's own a specification, IMO there is no way to distinguish the doc and the types -> If you have to tune the type you probably should not use the
@docstringhelper because you also have to tune the documentation.Checklist:
<Module or Topic>: <Action> <Summary>(see contributing guidelines)