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

Remove nullOk in MediaQuery.of #68736

Merged
merged 3 commits into from Oct 28, 2020
Merged

Remove nullOk in MediaQuery.of #68736

merged 3 commits into from Oct 28, 2020

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Oct 21, 2020 •
edited

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
goderbauer
goderbauer reviewed Oct 22, 2020
View reviewed changes
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
goderbauer
goderbauer approved these changes Oct 27, 2020
View reviewed changes
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 •
edited

@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 gspencergoog mentioned this pull request Oct 27, 2020
Open
@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
@ebot64 ebot64 mentioned this pull request Oct 29, 2020
Closed
@hillelcoren hillelcoren mentioned this pull request Nov 1, 2020
Closed
@krisu7 krisu7 mentioned this pull request Nov 1, 2020
Closed
@daniel-vera-g daniel-vera-g mentioned this pull request Nov 2, 2020
Closed
josh-burton added a commit to josh-burton/flutter_device_preview that referenced this pull request Nov 3, 2020
@josh-burton
Fixes compile errors due to using MediaQuery.of(context, nullOk)
2aa54e2 
This constructor has been replaced by a new MediaQuery.maybeOf() constructor.

flutter/flutter#68736
@josh-burton josh-burton mentioned this pull request Nov 3, 2020
Merged
@passsy passsy mentioned this pull request Nov 9, 2020
Closed
@BrayFlex BrayFlex mentioned this pull request Nov 19, 2020
Closed
@rydmike rydmike mentioned this pull request Jan 22, 2021
Closed
@wangfei-hz wangfei-hz mentioned this pull request Mar 4, 2021
Closed
@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
Reviewers

@Hixie Hixie Hixie left review comments

@goderbauer goderbauer goderbauer approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gspencergoog @goderbauer @Hixie @fluttergithubbot