Conversation
|
Latest documentation preview deployed successfully.
Note: This comment is updated whenever you push a commit. |
|
Web viewer built successfully.
Note: This comment is updated whenever you push a commit. |
emilk
changed the title
ImageFormat
emilk
changed the title
oxkitsune
left a comment
oxkitsune
left a comment
There was a problem hiding this comment.
LGTM!
Very nice improvement!
ntjohnson1
left a comment
ntjohnson1
left a comment
There was a problem hiding this comment.
I'm a little on the fence here so excuse the verbose post (accompanied by the approval since I don't think there is anything WRONG here).
- This probably will break lots of people's (and our own external repos)
- I don't generally see this pattern of
*to explicitly force people to switch to kwargs but it's not totally bizarre - I think the hope of
PLR0917is to reduce the total number of arguments and that this is an intermediate step to PLR0913 - I wonder if varying between 1 and 3 positional args will be weird to people, I guess if we choose the right args to expose should be fine
I definitely don't want KWARGS only but this is probably a reasonable compromise. PLR0913 is probably a much larger refactor but if we are feeling adventurous we could try enabling it, exclude all current offenders and hope its existence prevents future argument sprawl.
timsaucer
left a comment
timsaucer
left a comment
There was a problem hiding this comment.
From a code correctness perspective this looks good.
There is a separate question of whether it's the right thing to do since it impacting so many user facing APIs.
That being said, the updates on their side may be a tad annoying but are each individually straigh forward.
| if 2 < required.len() { | ||
| // There's a lot of required arguments. | ||
| // Using positional arguments would make usage hard to read. | ||
| // better for force kw-args for ALL arguments: | ||
| chain!(std::iter::once("*".to_owned()), required, optional).collect() | ||
| } else if optional.is_empty() { |
There was a problem hiding this comment.
It looks like this is going to set all arguments to kw-args if number of required args is 3 or more but down in the ruff rules we have max-positional-args = 3. So is 3 allowable or do you want to intentionally be more restrictive here or did I miss something about how this bit works?
There was a problem hiding this comment.
I wanted to be more restrictive here, at least for now. We seem to have no archetypes with exactly three required fields, so it doesn't matter much in practice for now. I rather start restrictive and expand later (removing a * is never a breaking change).
Co-authored-by: Nick <[email protected]>
4 participants
Using a lot of positional arguments have many downsides. One is readability, the other is future-proofing. If we want the ability to change our interfaces in the future, we need to force users to name the arguments.
I've written a migration guide.
None of our own examples or snippets needed updating, so I believe the impact on users will be rather small.
But this is really on of those "Best time to do it was three years ago, next best time is today"