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

UP007: opt out of Optional #4858

Open
Fogapod opened this issue Jun 5, 2023 · 9 comments · May be fixed by #11379
Open

UP007: opt out of Optional #4858

Fogapod opened this issue Jun 5, 2023 · 9 comments · May be fixed by #11379
Labels
help wanted Contributions especially welcome rule Implementing or modifying a lint rule

Comments

@Fogapod
Copy link

Fogapod commented Jun 5, 2023

Hello, currently UP007 covers both Union[a, b, c] -> a | b | c and Optional[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 rules
  • UP039 is added which only reports Optional

The 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 unchanged
  • UP039 is added which reports Union and later added rules

This might be even worse because

  1. for people with UP in config it does double work
  2. if someone had UP enabled and UP007 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 asottile/pyupgrade#390 (comment). don't know how much you are willing to deviate from "upstream" for cases like this but it would be a nice addition

@charliermarsh charliermarsh added the question Asking for support or clarification label Jun 8, 2023
@charliermarsh
Copy link
Member

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.

@lengau
Copy link
Contributor

lengau commented Jun 27, 2023

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).

@charliermarsh
Copy link
Member

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?

@lengau
Copy link
Contributor

lengau commented Jun 30, 2023

Not based on a formal survey, just based on asking other developers what they think

@lengau
Copy link
Contributor

lengau commented Jun 30, 2023

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.

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer and removed question Asking for support or clarification labels Jul 10, 2023
@ktbarrett
Copy link

ktbarrett commented Mar 12, 2024

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.

@zanieb
Copy link
Member

zanieb commented Mar 12, 2024

Sure. Someone can do this if they want:

  1. Drop Optional support from UP007 in preview mode
  2. Add new rule in preview for use of Optional
  3. Add documentation about the transition to both rules

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.

@zanieb zanieb added help wanted Contributions especially welcome and removed needs-decision Awaiting a decision from a maintainer labels Mar 12, 2024
@blueraft blueraft linked a pull request May 12, 2024 that will close this issue
3 tasks
@JelleZijlstra
Copy link
Contributor

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.

@ktbarrett
Copy link

ktbarrett commented May 14, 2024

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.

edwardzjl added a commit to edwardzjl/chatbot that referenced this issue Jul 11, 2024
edwardzjl added a commit to edwardzjl/chatbot that referenced this issue Jul 11, 2024
yugokato added a commit to yugokato/openapi-test-client that referenced this issue Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants