-
Notifications
You must be signed in to change notification settings - Fork 16
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
Better transformer API #489
base: master
Are you sure you want to change the base?
Conversation
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 have mixed feelings. I like the simplicity of interface and the reduction of boilerplate. But we are loosing consistency. Earlier everywhere first argument had to be TModule
. Now in transformer API it wouldn't be needed, but e.g. in method calls it will be still required. Nevertheless I am ready to approve such changes.
else: | ||
return func # type: ignore | ||
|
||
|
||
def def_helper(description, func: Callable[..., T], tp: type[U], arg: U, /, **kwargs) -> T: |
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.
Shouldn't be func
annotated as CallableOptParam
? I have the same question regarding mock_def_helper
and method_def_helper
.
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.
It could, but it basically changes nothing because of the ...
parameter specification.
I think we are actually gaining consistency, as:
Also:
|
This PR utilizes
def_helper
mechanisms to create a nicer API for using transformers. Following changes in interpreting the function arguments of the transformer classes are made:arg
) the functions can take individual record fields, just like methods defined viadef_method
.Also, specializations of
def_helper
were moved closer to their use sites, so that the dependencies on types would resolve correctly. If someone has a better idea, I'd like to hear it.To do: