Skip to content

Conversation

@WrRan
Copy link
Contributor

@WrRan WrRan commented Feb 27, 2025

simple fix:
not ... in ... => ... not in ...

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, but I'm also not sure if this counts as a cosmetic change. (I'd rather not inspire a bunch of copycat PRs switching not x in y to x not in y, depending on how many of those we have.)

@picnixz, do you know the rules/procedure here?

@vstinner
Copy link
Member

In general, we don't accept changes which are only "coding style" changes. But then if a core dev wants the change, why not. It's not a big deal. I would just suggest to not backport such coding style changes.

@eli-schwartz
Copy link
Contributor

I mean, obviously it should go without saying that this PR does not fix a regression, nor even a bug, so it makes no sense to backport it under any circumstances.

It is very much cosmetic either way. The only thing that cares about this change is a style linter. Personally, I would suggest accepting cosmetic style changes to codebases... but Python's contribution policy discourages such. And it feels a bit weird to say "no, don't do it" and then accept a PR if someone does in fact do it anyway.

@vstinner
Copy link
Member

And it feels a bit weird to say "no, don't do it" and then accept a PR if someone does in fact do it anyway.

We usually accept coding style changes in the middle of changes which fix bugs or add new features.

@picnixz
Copy link
Member

picnixz commented Mar 2, 2025

do you know the rules/procedure here?

For the modules I personally maintain and touch a lot, I don't mind when someone changes something like that because it's still ugly. For instance, we say that we accept changes when we touch the code around, but sometimes that code would never be touched in the future.

Now, in this case, argparse has seen lots of changes recently, and I think many were backported, so I think it's better to backport that if we were to apply that change. Since argparse is a module that still evolves (contrary to some modules like netrc or ftplib that are REALLY slow to update), I won't endorse that change as I fear that changing this one could introduce conflicts in future backports if we don't backport it. Now, we don't have a codeowner for argparse, so I leave the decision to those that contributed the most these past months, namely @serhiy-storchaka or @savannahostrowski

@serhiy-storchaka
Copy link
Member

We usually do not accept pure cosmetic changes, as it leads to code churn and encourages other changes that lead to more code churn. The style of this expression can be changed if that line or some of near lines be changed for other reasons, or in a global clean up performed by a maintainer (wery rarely).

@ZeroIntensity ZeroIntensity added the pending The issue will be closed if no feedback is provided label Mar 2, 2025
@picnixz
Copy link
Member

picnixz commented Mar 2, 2025

Considering that Victor seems neutral on that, that I am neutral as well, but that the main maintainer of argparse did not explicitly approve the PR, I suggest closing this one (the module evolves sufficiently fast that we can probably hit the line sometime in the future.

(Note that the last change around that line was 5 months ago so we could have change it at that time)

@vstinner
Copy link
Member

vstinner commented Mar 2, 2025

I'm against this change.

@picnixz
Copy link
Member

picnixz commented Mar 2, 2025

I'm against this change.

I guess that solves the pending label. Closing since no core dev endorsed that change while others weren't really convinced.

@picnixz picnixz closed this Mar 2, 2025
@ZeroIntensity ZeroIntensity removed awaiting review pending The issue will be closed if no feedback is provided labels Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants