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.

Sign up for GitHub

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

Jump to bottom

Use ... | None instead of Optional[...] #3856

Closed
suhaibmujahid opened this issue Nov 25, 2023 · 2 comments · Fixed by #3910
Closed

Use ... | None instead of Optional[...] #3856

suhaibmujahid opened this issue Nov 25, 2023 · 2 comments · Fixed by #3910
Labels
good-first-bug Good for newcomers

Comments

@suhaibmujahid
Copy link
Member

suhaibmujahid commented Nov 25, 2023 •
edited
Loading

We should make it more consistent. We could use ... | None instead of Optional[...].

Originally posted by @suhaibmujahid in #3855 (comment)
@suhaibmujahid suhaibmujahid added the good-first-bug Good for newcomers label Nov 25, 2023
@suhaibmujahid suhaibmujahid mentioned this issue Nov 25, 2023
Merged
@theoniko
Copy link
Contributor

theoniko commented Dec 9, 2023

Hello @suhaibmujahid,
Could i work on this issue?
Could you please exlain me why revs: list[bytes] | None = None is better than revs: Optional[list[bytes]] = None?
They seem to have similar effect to me i.e list of bytes or None. Is just for consistancy in code?

@suhaibmujahid
Copy link
Member Author

Could i work on this issue?

Yes!

Could you please exlain me why revs: list[bytes] | None = None is better than revs: Optional[list[bytes]] = None?
They seem to have similar effect to me i.e list of bytes or None. Is just for consistancy in code?

Yes, they are equivalents. The main reason is consistency. Also, Optional[] is a bit annoying because it requires to be imported.

kaijif added a commit to kaijif/bugbug that referenced this issue Dec 9, 2023
@kaijif
Change Optional[...] to ... | None (fixes mozilla#3856)
b57d638
@kaijif kaijif mentioned this issue Dec 9, 2023
Merged
@suhaibmujahid suhaibmujahid linked a pull request Dec 9, 2023 that will close this issue
Change Optional[...] to ... | None #3910
Merged
@suhaibmujahid suhaibmujahid mentioned this issue Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good-first-bug Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change Optional[...] to ... | None kaijif/bugbug
2 participants
@suhaibmujahid @theoniko