Skip to content

Conversation

@JelleZijlstra
Copy link
Member

@JelleZijlstraJelleZijlstra commented Sep 23, 2024

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

I was just surprised that updating func.__annotations__ after update_wrapper(wrapper, func) doesn't update wrapper.__annotations__. Don't get me wrong, I'm fine with this behavior.

Comment on lines 72 to 73
func=_get_get_annotations()
returnfunc(wrapped, format=format)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func=_get_get_annotations()
returnfunc(wrapped, format=format)
get_annotations=_get_get_annotations()
returnget_annotations(wrapped, format=format)

@vstinner
Copy link
Member

Another surprising behavior. I'm discovering __annotate__() so it's maybe by design, I don't know :-)

When I update annotations of the wrapped function, __annotate__() of the wrapped function doesn't change, whereas __annotate__() of the wrapper is updated.

$ ./python >>> import functools >>> def f(x: int = 1): return x >>> def wrapper(*args): pass >>> functools.update_wrapper(wrapper, f) <function f at 0x7f19f44d3e90> >>> f.__annotations__['x'] <class 'int'> >>> import annotationlib >>> f.__annotate__(annotationlib.Format.VALUE){'x': <class 'int'>} >>> wrapper.__annotate__(annotationlib.Format.VALUE){'x': <class 'int'>} >>> f.__annotations__['x'] = float >>> f.__annotate__(annotationlib.Format.VALUE) # <=== HERE{'x': <class 'int'>} >>> wrapper.__annotate__(annotationlib.Format.VALUE) # <=== HERE{'x': <class 'float'>} 

I would expect getting the same value for both cases. Either <class 'int'> or <class 'float'>.

@vstinner
Copy link
Member

cc @sobolevn

@JelleZijlstra
Copy link
MemberAuthor

This is because we use get_annotations, which always returns a fresh dictionary. Maybe I should change that.

@JelleZijlstra
Copy link
MemberAuthor

I made some changes to align with @ncoghlan's original proposal and @vstinner's feedback above, but I now think it's better to stick with what's currently on main. I'll write that up on the issue.

@rhettingerrhettinger removed their request for review September 25, 2024 22:22
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JelleZijlstra@vstinner