Open Fogapod opened 1 year ago
A third option would be to add a setting to retain Optional
, but keep all behavior under UP007
. I'm somewhat undecided on this overall though.
Just adding my voice to agree with this.
It appears that most people are either in favour of or indifferent to X | Y
replacing Union[X, Y]
, but a big chunk (roughly half) of those prefer to disable UP007
to keep Optional[Z]
over Z | None
. Personally I'm indifferent to the latter and have a moderate preference for X | Y
, so I'd prefer to be able to enforce the replacement of Union
without enforcing the replacement of Optional
, allowing the use of Optional
.
Breaking it up into two rules (or perhaps even three, leaving UP007
as the union of the two rules with an eventual deprecation, much like with #1980) is probably more intuitive than having it be a configuration on the rule. Despite Optional[X]
literally being the same as Union[X, None]
from a logical standpoint, from a readability and utility standpoint they are quite different (Optional
having a special enough meaning to have been distinguished from Union
in the first place). If there were a need to match upstream rules like in Pycodestyle I'd probably have the opposite opinion, but as pyupgrade bundles its rules by Python version a direct mapping is already out of the window (e.g. UP031 and UP032 being separate already).
I personally don't have a preference for Optional[a]
over a | None
but I'm not opposed to splitting these into separate rules. (I'd prefer separate rules over a setting.) My hesitation is that I'm not sure how to do that migration in a way that's minimally disruptive.
...but a big chunk (roughly half)...
Not challenging this, but is this based on data? Like a survey or something? Or your own experiences and interactions?
Not based on a formal survey, just based on asking other developers what they think
What if UP007 became a way to just enable both of the split out rules + write a deprecation warning?
I'm not too worried about the edge case where someone has UP
enabled but ignores UP007
specifically, as updates right now are well known to introduce new rules that will introduce failures, so nobody should be surprised by it (and the command output and documentation are both good enough to make it easy for people to understand and update as needed). If you are concerned about that edge case, you could treat having the ignore group include UP007 disable the other rules.
Whose decision is this waiting on? I think the first solution is the least disruptive and I would also like to see this rule split between Union
and Optional
as I much prefer Optional
: it's clearer documentation that the argument is optional to those not in the know. I'd also would not like to see recommendations in the other direction (to prefer Optional
over X | None
).
In my opinion Optional[X] = None
and X | None
(no default) encode two different idioms and are the best ways to spell those idioms.
Sure. Someone can do this if they want:
Optional
support from UP007
in preview modeOptional
I'm not too concerned about people ignoring UP007
right now encountering the new rule, if anything that will be a signal that they can enable UP007
again. It'll happen in a breaking release and we'll be able to gauge how much effect it will have when 2. is implemented.
it's clearer documentation that the argument is optional to those not in the know
But it isn't: Optional means the parameter may be None, not that it is optional. A parameter can be optional (i.e., have a default) without having an Optional type, and vice versa. This is actually one of the main reasons from my perspective why Optional should be avoided: its name is misleading.
Perhaps Optional
shouldn't mean the same thing as X | None
, but instead pass X, or don't give an argument, giving a warning otherwise. Not that I expect this to ever change... but explicitly passing None to optional arguments (which apparently has no spelling at all) is a code smell.
Hello, currently UP007 covers both
Union[a, b, c]
->a | b | c
andOptional[a]
->a | None
. Personally i find Optional to be more readable and PEP 604 does not deprecate this syntax either.I'm not sure how it could be changed without breaking changes. I see 2 ways:
UP007
stops reporting Optional, keeps reporting Union and later added rulesUP039
is added which only reports OptionalThe downside of this is that it forces people to update config to get previous behavior if they didn't have entire
UP
category enabled. Alternative is:UP007
remains unchangedUP039
is added which reports Union and later added rulesThis might be even worse because 1) for people with
UP
in config it does double work 2) if someone hadUP
enabled andUP007
ignored, the Union lint would reappear (although I'd assume most people who do this because of Optional)pyupgrade author made it clear in multiple issues that they are not going to change this https://github.com/asottile/pyupgrade/issues/390#issuecomment-1046896890. don't know how much you are willing to deviate from "upstream" for cases like this but it would be a nice addition