Skip to content
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

refactor: remove add_args_kwargs. #339

Merged
merged 3 commits into from
Feb 19, 2024
Merged

Conversation

paquiteau
Copy link
Contributor

@paquiteau paquiteau commented Feb 14, 2024

This module brings little value, and mostly nasty bug by adding extra layers.

We are all consenting adults, no need for enforcing *args and **kwargs

This is 1) expensive 2) dangerous 3) source of bug.
@chaithyagr
Copy link
Contributor

While this surely looks good to me, I will add Sam to review in case we missed some reason why this should exist.

@sfarrens
Copy link
Contributor

Hi @paquiteau this makes sense to me, however shouldn't check_callable then check if the input method does indeed support args and kwargs, because I think (I would need to double check everything) it will break some algorithms if a manually defined operator does not.

Also, if you want to fully clear out this functionality you will need to additionally make the following changes in types.py check_callable:

  • remove the add_agrs=True kwarg
  • update the Parameters section in the docstring
  • update the Returns section in the docstring
  • remove the See Also section in the docstring

P.S. "consenting adults" 🤣

@paquiteau
Copy link
Contributor Author

The checks for the *args and **kwargs is problematic if you consider all the @Property that have been defined, wrapping internal method (a typical examples are the _cost methods, wrapped in cost). The inspect module is not able to time finding the args and kwargs in the definition (which is what causes the nasty bugs mentioned above.

@sfarrens
Copy link
Contributor

@paquiteau all good, feel free to merge. :)

@paquiteau paquiteau merged commit d37e1e4 into CEA-COSMIC:master Feb 19, 2024
1 of 9 checks passed
@paquiteau paquiteau deleted the rm-args branch February 19, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants