Improve add_signature_parameters : add first_possible and last_possible

[smarie]

Sometimes adding a parameter at the beginning or end of parameters list would make the resulting signature invalid. Typically, adding a keyword-only parameter before a keyword-or-positional, or adding a parameter without default after a parameter with a default (and conversely)...

Two new arguments first_possible and last_possible could allow the users to ask for a "smart insert first" and "smart insert last" feature.

This would help solving smarie/python-pytest-cases#279
in a reusable way.

[vyasr]

Is there any scenario where you wouldn't want last to behave like last_possible? It seems to me like if we were to add these two new options rather than modifying the existing ones, then at minimum we would want first and last to throw errors if the resulting signatures would be ill-formed. At that point is there much benefit to not just making this behavior the default for first and last?

[vyasr]

On second thought, I am rethinking my position. Explicit is better than implicit after all. I think changing first and last to throw errors along with this change would be sensible, but automatically remapping the position may be a bit more magic than we want by default.

[smarie]

On second thought, I am rethinking my position. Explicit is better than implicit after all.

Yes this is basically what I had in mind: users thinking that first is safe in their use case would be interested in an error being raised, instead of some magic to happen. Now should we raise our own variant of python's base SyntaxError ? My guess is that this is not useful, as the latter has a message that is already ok to understand.

[vyasr]

Now should we raise our own variant of python's base SyntaxError ? My guess is that this is not useful, as the latter has a message that is already ok to understand.

You are correct. I was still in the mindset of smarie/python-pytest-cases#279, where the current error is not easy to understand. However, that is because it is an internal issue with pytest-cases creating invalid signature that are opaque to the user of pytest-cases. If we update pytest-cases to use the new last_possible feature then everything should be fine.

[vyasr]

How should this be handled in the case where parameters are of different kinds? It's fairly obvious how last_possible should behave when all the parameters are positional, for instance, but what happens if some are positional and some are keyword? Do the positional ones go at the end of the list of positional parameters, while the keyword ones go at the end of the kwargs list? Should these work with functions containing positional-only or keyword-only parameters? I'm not sure that there's any way to define this API that isn't surprising in at least some cases, so we may want to restrict its range of applicability.

[smarie]

Do the positional ones go at the end of the list of positional parameters, while the keyword ones go at the end of the kwargs list? Should these work with functions containing positional-only or keyword-only parameters?

Yes, I guess that the "conctract" should be based on the most intuitive behaviour for users. So for example

• last_possible: each parameter will be inserted at the right-most possible position in the signature, while preserving Syntactic validity. When possible, the initial order of provided parameters is preserved.

So I guess the easiest way to proceed would be to handle each parameter one by one

[vyasr]

I can implement this, but tbh I'm not sure how valuable this feature will be. I think we may be better off resolving smarie/python-pytest-cases#279 directly in pytest-cases rather than trying to add this functionality. The problem that I see is that, except in very special cases like pytest tests where pytest is handling function parameters via its own dependency injection mechanisms, it is generally not very useful to modify a function signature without knowing the exact desired result a priori. Wouldn't it make more sense to put the onus on the calling code to figure out what the first or last valid position is? That way the calling code also actually knows what valid ways to call the function are. Otherwise, there's no guarantee that a user creating a function in this way will have enough information about the signature to use it correctly afterwards. For instance, they could easily pass parameters in the wrong order.

[lucaswiman]

I think adding a keyword-only argument with a default is almost always safe, unless there is an existing argument of the same name. Supporting that case seems useful. It also seems reasonable that makefun would figure out where it's safe to insert an argument and put it there.

[vyasr]

I agree, this could be made to work for a keyword-only parameter. My concern applies more to positional or positional-or-keyword parameters, where the calling code can't make assumptions about the resulting API so it doesn't seem very useful. Maybe we should implement this and just restrict this to keyword-only parameters, then. Does that sound reasonable to both of you @lucaswiman @smarie?

If we do go in that direction, this won't really address the original problem in smarie/python-pytest-cases#279, but I think that's OK.

[smarie]

Thanks to both of you ! To be honest I think that it would be a little sad not to solve the original issue here.

It seems to me that trying to restrict the scope of the feature will actually make the implementation (and documentation) more difficult to do.

For last_possible the pseudo code could be :

position to insert = 0
for p in last_possible:
    if not _are_params_valid_in_this_order(current_params[position_to_insert + 1], p):
        # we can not insert p at `position_to_insert + 1`, let's insert it at position `position_to_insert`
        break
    else:
        position_to_insert += 1

and the "secret sauce" could be in _are_params_valid_in_this_order (comparing the kind of the two parameters to decide if the first one can be defined before the second one)

Maybe one way to start could be to write a few tests first, to be sure we agree on what this should do ?

@vyasr if solving the initial pytest-cases issue is the priority on your side, then I suggest that you create a PR directly in pytest-cases, that would solve only that specific situation, so that we have a bit more time to elaborate a PR here in makefun all together. Indeed in makefun there is not much emergency in my opinion. What do you think ?

[vyasr]

No worries, my workaround is perfectly sufficient for now so I'm happy to try to resolve this first.

Maybe one way to start could be to write a few tests first, to be sure we agree on what this should do ?

Yes, this is how I started and what made me ask all these questions. I am happy to put together an implementation early next week in a draft PR that we can iterate on.