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

Add UP007 to ruff ignore list. #337

Closed
wants to merge 3 commits into from
Closed

Add UP007 to ruff ignore list. #337

wants to merge 3 commits into from

Conversation

alexjbuck
Copy link
Contributor

@alexjbuck alexjbuck commented Dec 19, 2023

We should prefer Optional[T] to T | None. It is a more expressive statement and semantically is more appropriate for many cases with optional parameters.

Rule UP007 was originally designed to prefer T | None over Union[T,None] which makes sense. It has not been updated to account for the relatively new Optional[T] syntax.

Test plan (required)

Tested locally adding "UP007" to ruff.toml ignore. Ruff ceases false positive lints against Optional[T] syntax.

Closing issues

closes #336

We should prefer `Optional[T]` to 'T | None`.
@alexjbuck alexjbuck requested a review from psirenny December 19, 2023 18:23
@alexjbuck alexjbuck self-assigned this Dec 19, 2023
Copy link

changeset-bot bot commented Dec 19, 2023

🦋 Changeset detected

Latest commit: b2e8dc0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spear-ai/ruff-config Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@psirenny
Copy link
Contributor

psirenny commented Dec 19, 2023

The optimal linter settings should try to reduce the number of different ways the same code could be written. If we disable this rule, then int | None, Union[int, None] and Optional[int] are all possible variations developers could (probably will) add which, is not ideal for consistency. It's hard to detect so it likely wouldn't get picked up in code review.

I think on net, disabling the rule is probably worse. Although… I do admit that I'm not bothered by the union syntax 😆. Python doesn't have a concept of undefined, so the union syntax seems more accurate to me. That's probably my TypeScript bias leaking through though.

@alexjbuck
Copy link
Contributor Author

I dislike the details of the outcome (I have to change a lot of Optional[T] occurrences). Optional creates the language concept that something can exist, or it can be None. I wouldn't say that undefined is the natural concept for a missing/non-existent Optional value. Even in Rust the Option<T> type is Some<T> or None.

However, I can get behind the idea of having one happy path for writing type hints, to increase consistency. I dislike Union types, why should a function be able return a Union[str, int]?? That's runtime madness. Anyway, I'll close this out and we can keep as is, such that T | None is the accepted way to write this, at least until UP007 is fixed to prefer Optional[T] for any T | None or Union[T, None] types,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude UP007 from Ruff Config, i.e. prefer Optional[T] over T | None
2 participants