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

Change definition to class Optional<T extends Object> #736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Jul 10, 2023

This prevents Optional from being declared with a nullable type. It is never useful to do so, because trying to create an instance with a null causes a run-time error anyway. So just confusing.

Internally we already applied this change and were quite easily able to clean up any existing occurrences.

This excludes nullable types from being able to be put into an Optional, which is confusing.
@cbracken
Copy link
Member

cbracken commented Jul 10, 2023

See for the last time we did this: #667.

If we've already done this internally though, that's great news and eliminates the main reason we backed this change out. It may be useful to conduct a quick use of open source projects to see what impact we can expect to users. Overall though, lgtm in principle from me.

@oprypin
Copy link
Member Author

oprypin commented Jul 10, 2023

See: #667

A survey of the Google codebase suggests this would be a fairly extensive breaking change.

It is a breaking change but as I said I already applied this change there. So if Google codebase was a big part of the reason, it no longer applies.

@cbracken
Copy link
Member

Apologies -- submitted the comment accidentally while I was still writing it. It's updated now.

@oprypin
Copy link
Member Author

oprypin commented Jul 10, 2023

It may be useful to conduct a quick use of open source projects to see what impact we can expect to users.

Hm I haven't done such a survey before. Maybe if someone already has a setup for something like this, they could chip in.

@cbracken
Copy link
Member

@yjbanov I think you've got experience doing this previously using BigQuery? If not, it's something the Flutter framework team has a lot of experience with. Will see if I can dig up a doc.

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.

None yet

2 participants