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

New option in decorator to prepend positional arguments #58

Closed
wants to merge 3 commits into from

Conversation

smarie
Copy link
Contributor

@smarie smarie commented Oct 26, 2018

This still needs a review and possibly improvements (such as also removing arguments by name and adding tests). Nevertheless it shows how it could be done.

Fixes #55

@smarie smarie changed the title New option in decorator to prepend positional arguments [do not MRG before #57] New option in decorator to prepend positional arguments Oct 26, 2018
@micheles
Copy link
Owner

micheles commented Nov 4, 2018

I am in doubt about this pull request, since it goes against the philosophy of the decorator module: its goal is to preserve the signature, not to change it. Having said that, I recognize that there are use cases where it would be handy to add extra arguments (I have one myself), so I am in doubt. What kind of syntax do you have in mind? Something like this?

@add_argument('request')
def func(arg1, arg2):
    ....

I would like to see the examples you have in mind.

@smarie
Copy link
Contributor Author

smarie commented Nov 5, 2018

I agree that this is not in the original philosophy of decorator module, but rather a "goodie" leveraging the fact the function maker capability.

The use case I have in mind was presented in #55, but it seems that I selected an overly complex one, making it a bit hard to catch. I added a simpler one in the thread, that will hopefully clarify the need.

… the user experience is always the same, even if some additional arguments are already present in f signature.
@smarie smarie changed the title [do not MRG before #57] New option in decorator to prepend positional arguments New option in decorator to prepend positional arguments Nov 7, 2018
@smarie
Copy link
Contributor Author

smarie commented Nov 7, 2018

As mentioned in #55, I updated the PR so that the behaviour is consistent for users, whatever the case (if additional_args contain arguments that are present in f, or not)

smarie added a commit to smarie/python-pytest-steps that referenced this pull request Nov 7, 2018
smarie added a commit to smarie/python-pytest-harvest that referenced this pull request Nov 9, 2018
@micheles
Copy link
Owner

I thought a lot about this patch, but there are 150 lines of changes - in a module which is meant to be small - for a functionality which is actually against the goal of preserving the function signature. So for now I am closing this PR. I may change my mind in the future, but for the moment I prefer to keep things simple.

@micheles micheles closed this Jan 24, 2019
@smarie
Copy link
Contributor Author

smarie commented Jan 30, 2019

I had some afterthought about this: as you rightfully explained on the issue page, the problem is that I try to use only the function maker part, but not in the context of decorating functions to reach identical signature - rather to create new functions with possibly different signatures.

For this reason maybe it would be better to create an independent library with the purpose to generate functions dynamically, whatever they are. There would be several ways to specify the desired signature, for example

  • providing a string containing code
  • providing a Signature object, potentially acquired by inspecting another function

Decorators would then only be a specific case of usage of this lib. What would you think of this ? I will experiment to see if I can provide an illustration of what I have in mind.

@micheles
Copy link
Owner

micheles commented Jan 30, 2019

I have no issues if you publish a new library with the new features you have in mind. The decorator module has a BSD license, so you have very few obligations to fullfill.

@smarie
Copy link
Contributor Author

smarie commented Jan 30, 2019

Thanks for the feedback! Ok then, I started something here, we'll see how that can go.

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.

None yet

2 participants