-
Notifications
You must be signed in to change notification settings - Fork 609
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: standardize use of keyword/positional-only arguments #9125
Comments
Opening this in favor of #9383 -- that PR also included all of the breaking changes to unify function signatures and it was too much at once. This PR adds only the signature checking mechanism, plus the requisite xfails to lay out which inconsistencies are currently in Ibis. ## Motivation We want to ensure that, for a given backend, that the argument names, plus usage of positional, positional-only, keyword, and keyword-only arguments match, so that there is API consistency when moving between backends. I've grabbed a few small parts of some of the utilities in Scott Sanderson's `python-interface` project (https://github.com/ssanderson/python-interface). While the upstream is no longer maintained, the goal of that project aligns quite well with some of the issues we face with maintaining consistent interfaces across backends. Note that while the upstream project focused on _runtime_ enforcement of these signatures matching, here it is only run in the test suite. ## Rough procedure Any method that doesn't match can be skipped entirely (this is useful for things like `do_connect`, which cannot reasonably be assumed to match) or individually (by specifying a `pytest.param` and marking the failing backends). Then we scrape across the common parent classes and add any methods that are NOT currently specified in the pre-existing xfailed ones. It's a bit of a nuisance, but it's done, and ideally the manual listing of the inconsistent methods goes away as we unify things. I've opted for not checking that type annotations match, because that seems... unreasonable. This would satisfy #9125 once all of the xfail markers are removed, e.g., it checks that all keyword and positional arguments are standardized.
@gforsyth is the implementation of this one a requirement for 10.0? |
At the very least, we should, I think, make all the breaking changes to positional argument names so that they are consistent across backends. |
…-project#10008) Opening this in favor of ibis-project#9383 -- that PR also included all of the breaking changes to unify function signatures and it was too much at once. This PR adds only the signature checking mechanism, plus the requisite xfails to lay out which inconsistencies are currently in Ibis. ## Motivation We want to ensure that, for a given backend, that the argument names, plus usage of positional, positional-only, keyword, and keyword-only arguments match, so that there is API consistency when moving between backends. I've grabbed a few small parts of some of the utilities in Scott Sanderson's `python-interface` project (https://github.com/ssanderson/python-interface). While the upstream is no longer maintained, the goal of that project aligns quite well with some of the issues we face with maintaining consistent interfaces across backends. Note that while the upstream project focused on _runtime_ enforcement of these signatures matching, here it is only run in the test suite. ## Rough procedure Any method that doesn't match can be skipped entirely (this is useful for things like `do_connect`, which cannot reasonably be assumed to match) or individually (by specifying a `pytest.param` and marking the failing backends). Then we scrape across the common parent classes and add any methods that are NOT currently specified in the pre-existing xfailed ones. It's a bit of a nuisance, but it's done, and ideally the manual listing of the inconsistent methods goes away as we unify things. I've opted for not checking that type annotations match, because that seems... unreasonable. This would satisfy ibis-project#9125 once all of the xfail markers are removed, e.g., it checks that all keyword and positional arguments are standardized.
@gforsyth It's not 100% clear to me whether the goal here was to make expression APIs more strict (from the original issue description it sounds like it could be), or whether based your statement we should tackle backend APIs for this as well (or perhaps first?). I went ahead did the expression API version, which will probably be more contentious than backend APIs: #10734 I was thinking that in the interest of getting a release out this week, we should avoid doing the backend APIs, and save those for 11.0.0 (which I don't think we need to wait to long to do). |
Python 3.8+ supports both keyword-only and positional-only arguments. We might want to survey our existing APIs and try to consistently make use of positional and keyword-only arguments in cases where they make sense.
They main way this would show up as an improvement is making it easier for us to refactor internals without making things a (potentially) breaking change for users. Say we had an existing function:
With this, users can call this in a few different formats:
Allowing these different calling conventions makes amending this API trickier. We have to worry about adding new keyword arguments only at the end, and never renaming any of the positional arguments (since the user may have called them by name). Moving to a stricter positional-only/keyword-only subset would mean that:
The text was updated successfully, but these errors were encountered: