-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT (SLEP6) remove other_params from provess_routing #26909
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
MNT (SLEP6) remove other_params from provess_routing #26909
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.
During my initial SLEP006 review, I have found other_params
a bit weird. I am happy with this change.
Does all the calls in process_routing
in examples/miscellaneous/plot_metadata_routing.py
need to change too?
@@ -1412,7 +1412,7 @@ def get_metadata_routing(self): | |||
# given metadata. This is to minimize the boilerplate required in routers. | |||
|
|||
|
|||
def process_routing(obj, method, other_params, **kwargs): | |||
def process_routing(_obj, _method, /, **kwargs): |
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.
Given that this is not the norm, may you add a comment here that explains the purpose of _obj
and _method
based on #26909 (comment) ?
Nice hint with the example usage, fixed. |
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.
Looks good. Thanks @adrinjalali
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.
LGTM
This PR removes the
other_params
fromprocess_routing
, since they can be passed as**kwarg
anyway. There used to be some difference between args passed explicitly and the ones passed as kwargs to methods such asfit
, but later with the introduction of a more richMetadataRouter
the need for that distinction was removed.This also renames
obj
to_obj
andmethod
to_method
as arguments and makes them positional only. This also reflects quite a bit from Python's API. The rename helps with avoiding collisions between metadata andobj
andmethod
as metadata names.cc @thomasjpfan @OmarManzoor