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

Remove nullOk in MediaQuery.of #68736

Merged
merged 3 commits into from
Oct 28, 2020
Merged

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Oct 21, 2020

Description

Adds MediaQuery.maybeOf to replace calling MediaQuery.of(context, nullOk: true), and removes the nullOk parameter. Also changes MediaQuery.of to return a non-nullable value, and removes many instances of the ! operator, reducing the possible places where a null dereference could occur.

Related Issues

Tests

  • No tests were harmed in the making of this PR.
  • Added a test to make sure that maybeOf works as expected, updated test output for media_query_test.dart and others.

Breaking Change

  • Yes, this is a breaking change that will require a migration guide.

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Oct 21, 2020
@google-cla google-cla bot added the cla: yes label Oct 21, 2020
@gspencergoog gspencergoog changed the title Remove null ok Deprecate/disable nullOk in MediaQuery.of Oct 21, 2020
@gspencergoog gspencergoog force-pushed the remove_null_ok branch 3 times, most recently from a3f89ee to 08827b6 Compare October 21, 2020 20:46
@gspencergoog gspencergoog marked this pull request as ready for review October 21, 2020 21:15
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. Since this is failing google3 I would argue that this is a breaking change, though, and we should probably publish a quick migration guide.

@gspencergoog
Copy link
Contributor Author

I agree, I'll work on the guide.

@goderbauer
Copy link
Member

@bwilkerson This would be another great use case for a flutter fix that automatically migrates user code.

@gspencergoog
Copy link
Contributor Author

Here's the design doc for the migration.

Writing the migration guide itself next.

@gspencergoog gspencergoog force-pushed the remove_null_ok branch 2 times, most recently from 9745730 to 46010dd Compare October 23, 2020 17:30
@gspencergoog
Copy link
Contributor Author

OK, I just removed the nullOk entirely, since I don't like the fact that we're avoiding a compile time check in order to fail at runtime: that's a much more error-prone failure mode, and will definitely surprise people: I'd rather it just fail to compile.

@Hixie
Copy link
Contributor

Hixie commented Oct 24, 2020

It's a pity we lose the analyzer warning that way.

@gspencergoog gspencergoog changed the title Deprecate/disable nullOk in MediaQuery.of Remove nullOk in MediaQuery.of Oct 27, 2020
@flutter-dashboard flutter-dashboard bot added d: examples Sample code and demos team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Oct 27, 2020
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Oct 27, 2020

@Hixie Well, we lose the analyzer warning, but gain a compiler error. I agree, is a pity that there's no way to say "Fail to compile, but give a custom message like a deprecation" for when someone uses a removed parameter.

Hopefully the docs at the site are clear enough that they'll understand what to do when they get the compiler error.

@Hixie
Copy link
Contributor

Hixie commented Oct 27, 2020

Might be worth following up with @leafpetersen to see if we can change Dart in some way in the future to make this kind of thing better.

@gspencergoog
Copy link
Contributor Author

OK, I submitted dart-lang/sdk#43942 as a feature request.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Mac framework_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Mac framework_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@gspencergoog gspencergoog merged commit 5528932 into flutter:master Oct 28, 2020
josh-burton added a commit to josh-burton/flutter_device_preview that referenced this pull request Nov 3, 2020
This constructor has been replaced by a new MediaQuery.maybeOf() constructor.

flutter/flutter#68736
@gspencergoog gspencergoog deleted the remove_null_ok branch October 7, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants