Skip to content

Upstream web support for IterableProperty<double>#37515

Merged
ferhatb merged 2 commits intoflutter:masterfrom
ferhatb:master
Aug 5, 2019
Merged

Upstream web support for IterableProperty<double>#37515
ferhatb merged 2 commits intoflutter:masterfrom
ferhatb:master

Conversation

@ferhatb
Copy link
Copy Markdown
Contributor

@ferhatb ferhatb commented Aug 2, 2019

Description

table_rendering_test uses IterableProperty which fails on web platform due to default double.toString() behavior. This PR updates diagnostics to use debugFormatDouble to support web platform.

Related Issues

#37491

Tests

I added a test in diagnostics_test.dart for IterableProperty.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 2, 2019
@ferhatb ferhatb requested review from jacob314 and yjbanov August 2, 2019 21:04
@ferhatb ferhatb added the platform-web Web applications specifically label Aug 2, 2019
return sb.toString();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not support this without forking the cases for regular and double properties?
If you wrote

Iterable<String> formattedValues = value.map((T v) { return v is double ? debugFormatDouble : v.toString(); });

Then you could reuse the existing logic for formatting the output in both the line break and regular cases. Someone might add an additional customization for formatting iterables in the future as the current options aren't always ideal so it would be nice to keep one copy of that code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Triggering the transform on whether T== double would also be fine with me. E.g

Iterable formattedValue = T == double ? value.map(debugFormatDouble) : value;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestion. I had tried value.map(debugFormatDouble) but it complained about closure, somehow T == double type check didn't flow through.

Very hard to pass flutter lint rules on this one since v as double is not allowed and T == double keeps v's type as T so you can pass without casting. So (T == double && v is double) was only way to unify since "v is double" won't work on web platform since int and double use num.

I updated the code to a simplified version.

@jacob314
Copy link
Copy Markdown
Contributor

jacob314 commented Aug 2, 2019

lgtm

}
return '[${sb.toString()}]';
final Iterable<String> formattedValues = value.map((T v) {
if (T == double && v is double) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@munificent It is unfortunate Flutter lints are requiring this check when all we want to do is check.
Can we get the T == double check to flow through or correct the Flutter lints to not worry about sealed types like double?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checking the type parameter for each element doesn't make much sense to me. It's a property of the surrounding class, so it's going to be the same each time. Why not just do if (v is double)?

That accomplishes everything you're trying to express. If you really want to check the type argument, you could do:

Iterable<String> formattedValues;
if (T == double) {
  formattedValues = value.map((T v) => debugFormatDouble(v as double));
} else {
  formattedValues = value.map((T v) => v.toString());
}

But you still have to do some kind of runtime type check per element because Dart doesn't have anything like template specialization.

@ferhatb ferhatb merged commit 7c27142 into flutter:master Aug 5, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants