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: standardize use of keyword/positional-only arguments #9125

Open
jcrist opened this issue May 6, 2024 · 2 comments
Open

refactor: standardize use of keyword/positional-only arguments #9125

jcrist opened this issue May 6, 2024 · 2 comments
Labels
breaking change Changes that introduce an API break at any level refactor Issues or PRs related to refactoring the codebase
Milestone

Comments

@jcrist
Copy link
Member

jcrist commented May 6, 2024

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:

def my_excellent_function(arg1, arg2=None, arg3=None):
   ...

With this, users can call this in a few different formats:

my_excellent_function(1)  # positional with defaults
my_excellent_function(1, 2, 3)  # all as positionals
my_excellent_function(arg1=1, arg2=2, arg3=3)  # all as keywords
my_excellent_function(1, 2, arg3=3)  # mix of positional and keyword

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:

  • We can change the names of positional arguments later without worrying that users may have passed them by keyword
  • We can reorder keyword arguments without worrying that users may have have passed them positionally
@jcrist jcrist added refactor Issues or PRs related to refactoring the codebase breaking change Changes that introduce an API break at any level labels May 6, 2024
gforsyth added a commit that referenced this issue Sep 13, 2024
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.
@ncclementi
Copy link
Contributor

@gforsyth is the implementation of this one a requirement for 10.0?

@gforsyth
Copy link
Member

At the very least, we should, I think, make all the breaking changes to positional argument names so that they are consistent across backends.

@ncclementi ncclementi added this to the 10.0 milestone Sep 24, 2024
ncclementi pushed a commit to ncclementi/ibis that referenced this issue Sep 24, 2024
…-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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that introduce an API break at any level refactor Issues or PRs related to refactoring the codebase
Projects
Status: backlog
Development

No branches or pull requests

3 participants