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

Make most keyword parameters invalid as positional arguments #1130

Open
rmjarvis opened this issue Jul 13, 2021 · 0 comments
Open

Make most keyword parameters invalid as positional arguments #1130

rmjarvis opened this issue Jul 13, 2021 · 0 comments
Labels
cleanup Non-functional changes to make the code better

Comments

@rmjarvis
Copy link
Member

Now that we don't have to support Python 2.7 anymore, we can start using the * in function signatures to indicate keyword-only arguments. IMO, this tends to make user code better, forcing them to self-document what things mean by naming parameters, rather than let them be cryptically just False or 3.5, etc.

In general, I think we should make almost everything keyword-only. The main exceptions are for the first argument of a function, when the meaning of that argument is clear from the name of the function. e.g. image.addNoise(noise) or obj.setFlux(flux). Occasionally there are cases in GalSim code where both of the first two arguments are fairly obvious from context. e.g. obj.shift(x,y).

The other main exception will be for our "leading underscore" functions. I don't know what the latest performance difference is between args and kwargs, but it at least used to be the case that kwarg parsing was slower (cf. https://bugs.python.org/issue27574). This makes some sense, since it requires some string parsing rather than just counting items in a list. I don't know how much of a difference it makes in practice, but since our leading underscore functions are designed to whittle away as much overhead as possible for performance, let's not add any back by making them use kwargs.

Beyond these kinds of cases, I think we should usually favor forcing the user to explicitly name any parameters they are using. But there may be some other exceptions here and there where it might make sense to allow a couple parameters to remain positional. I suspect we can probably use our own code (tests, examples, and internal calls) as a guide for when to allow positional arguments. If we found it clear enough to write something without a keyword, we might probably want to let users do so too.

This is technically an API change, but I have a decorator we can use during the 2.x series to make it warn for violations rather than raise TypeErrors, which I wrote for the corresponding effort for TreeCorr. cf. https://github.com/rmjarvis/TreeCorr/pull/129/files#diff-56ac2f79094c0869fb8bbe6744b6cbcd7e270e9f6cac90b9373993ec2c14524fR927

@rmjarvis rmjarvis added the cleanup Non-functional changes to make the code better label Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Non-functional changes to make the code better
Projects
None yet
Development

No branches or pull requests

1 participant