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

Prefer void to null #22977

Merged
merged 4 commits into from
Oct 16, 2018
Merged

Prefer void to null #22977

merged 4 commits into from
Oct 16, 2018

Conversation

a14n
Copy link
Contributor

@a14n a14n commented Oct 11, 2018

No description provided.

@a14n a14n requested a review from Hixie October 12, 2018 13:13
Hixie
Hixie reviewed Oct 12, 2018
View reviewed changes
packages/flutter_test/test/widget_tester_test.dart Outdated
@@ -625,7 +625,7 @@ class FakeMatcher extends AsyncMatcher {

@override
Future<String> matchAsync(dynamic object) {
return completer.future.then<String>((void _) {
return completer.future.then<String>((_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be void value

Hixie
Hixie reviewed Oct 12, 2018
View reviewed changes
packages/flutter/lib/src/widgets/binding.dart Outdated
deferFirstFrameReport();
if (renderViewElement != null)
buildOwner.reassemble(renderViewElement);
return super.performReassemble().then((Null value) {
return super.performReassemble().then((void _) {
Copy link
Contributor

Choose a reason for hiding this comment

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

void value

Hixie
Hixie reviewed Oct 12, 2018
View reviewed changes
packages/flutter/lib/src/widgets/animated_list.dart Outdated
@@ -332,7 +332,7 @@ class AnimatedListState extends State<AnimatedList> with TickerProviderStateMixi
..sort();
});

controller.reverse().then<void>((Null value) {
controller.reverse().then<void>((_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

void value here and above

Hixie
Hixie reviewed Oct 12, 2018
View reviewed changes
packages/flutter/lib/src/material/scaffold.dart Outdated
@@ -1192,7 +1192,7 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin {
_snackBarController.value = 0.0;
completer.complete(reason);
} else {
_snackBarController.reverse().then<void>((Null _) {
_snackBarController.reverse().then<void>((_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

void value

Hixie
Hixie reviewed Oct 12, 2018
View reviewed changes
packages/flutter/lib/src/material/reorderable_list.dart Outdated
@@ -301,7 +301,7 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T
scrollOffset < bottomOffset ? bottomOffset : topOffset,
duration: _scrollAnimationDuration,
curve: Curves.easeInOut,
).then((Null none) {
).then((void none) {
Copy link
Contributor

Choose a reason for hiding this comment

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

void value

Hixie
Hixie reviewed Oct 12, 2018
View reviewed changes
packages/flutter/lib/src/material/refresh_indicator.dart Outdated
@@ -328,7 +328,7 @@ class RefreshIndicatorState extends State<RefreshIndicator> with TickerProviderS
_mode = _RefreshIndicatorMode.snap;
_positionController
.animateTo(1.0 / _kDragSizeFactorLimit, duration: _kIndicatorSnapDuration)
.then<void>((Null value) {
.then<void>((_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

void value

Hixie
Hixie reviewed Oct 12, 2018
View reviewed changes
packages/flutter/lib/src/cupertino/button.dart Outdated
? _animationController.animateTo(1.0, duration: kFadeOutDuration)
: _animationController.animateTo(0.0, duration: kFadeInDuration);
ticker.then<void>((Null value) {
ticker.then<void>((_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

void value

@Hixie
Copy link
Contributor

Hixie commented Oct 12, 2018

LGTM modulo minor nits
please update changelog accordingly

@a14n
Copy link
Contributor Author

a14n commented Oct 13, 2018

please update changelog

Where can I find the changelog?

Regarding (_) it seems that always_specify_types doesn't lint lambdas.

@a14n a14n added the c: API break Backwards-incompatible API changes label Oct 15, 2018
@goderbauer
Copy link
Member

You can find the changelog in the wiki. Here's the direct link: https://github.com/flutter/flutter/wiki/Changelog

@a14n
Copy link
Contributor Author

a14n commented Oct 15, 2018

@goderbauer thanks. @Hixie also pointed https://flutter.io/design-principles/#handling-breaking-changes

@a14n a14n merged commit 0fb84e9 into flutter:master Oct 16, 2018
@a14n a14n deleted the prefer_void_to_null branch October 16, 2018 20:04
@dnfield
Copy link
Contributor

dnfield commented Oct 16, 2018

@a14n - thanks for the contributions. flutter-build was red when this was landed - was this meant to fix the build? There's another PR that was meant to do that that's still going through the test cycles to make the tree green again (we hope).

@a14n
Copy link
Contributor Author

a14n commented Oct 16, 2018

@dnfield sorry I didn't notice it was red 👎 Should I revert this PR ?

@a14n
Copy link
Contributor Author

a14n commented Oct 16, 2018

Actually I only have access to a small number of tasks in https://flutter-dashboard.appspot.com/build.html and I'm not able to see the cause when it's red. (small excuse: Circus was green when I merged this PR)

@dnfield
Copy link
Contributor

dnfield commented Oct 16, 2018

No problem, I know it's confusing and sometimes the CI reporting isn't even up to date. I'd leave it for now.

For future reference, even if Cirrus is green we want flutter-build to be green as well - those are all checks done post commit and run more than just what Cirrus does. That way we're sure not to pile more to bisect through if we have to revert.

@a14n
Copy link
Contributor Author

a14n commented Oct 16, 2018

I'm not able to see the cause when it's red

For instance my commit makes the task channels_integration_test_win red but I cannot see why.

For future reference, even if Cirrus is green we want flutter-build to be green as well - those are all checks done post commit and run more than just what Cirrus does. That way we're sure not to pile more to bisect through if we have to revert.

Ack. Thanks for the information and sorry again for this too early merge

@dnfield
Copy link
Contributor

dnfield commented Oct 16, 2018

Your commit didn't make channels_integration_test, it was a prior commit doing that. flutter-build looks backwards at landed commits.

@dnfield
Copy link
Contributor

dnfield commented Oct 16, 2018

Ahh I take it back. It did make it red. Unfortunately, those logs are only available internally here.

It was a timeout though, I'm going to rerun - don't think it's related to your code.

@dnfield
Copy link
Contributor

dnfield commented Oct 16, 2018

Yup was just a flake, think we should be good now - thanks again!

@a14n
Copy link
Contributor Author

a14n commented Oct 16, 2018

Thanks to you :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Hixie Hixie Hixie left review comments

Assignees
No one assigned
Labels
c: API break Backwards-incompatible API changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@a14n @Hixie @goderbauer @dnfield @googlebot