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

[palette_generator] Migrate to null safety #287

Merged
merged 40 commits into from
Mar 10, 2021

Conversation

mzdm
Copy link
Contributor

@mzdm mzdm commented Feb 14, 2021

Related tracking issue flutter/flutter#75567

@mzdm

This comment has been minimized.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

I did a quick initial pass, but @gspencergoog should do the full review.

packages/palette_generator/CHANGELOG.md Outdated Show resolved Hide resolved
packages/palette_generator/example/lib/main.dart Outdated Show resolved Hide resolved
packages/palette_generator/example/lib/main.dart Outdated Show resolved Hide resolved
packages/palette_generator/example/lib/main.dart Outdated Show resolved Hide resolved
packages/palette_generator/lib/palette_generator.dart Outdated Show resolved Hide resolved
packages/palette_generator/lib/palette_generator.dart Outdated Show resolved Hide resolved
packages/palette_generator/lib/palette_generator.dart Outdated Show resolved Hide resolved
packages/palette_generator/lib/palette_generator.dart Outdated Show resolved Hide resolved
@mzdm mzdm marked this pull request as ready for review February 26, 2021 20:46
As you mentioned - that's now expressed in the type.
This reverts commit 0855080
packages/palette_generator/example/lib/main.dart Outdated Show resolved Hide resolved
packages/palette_generator/example/lib/main.dart Outdated Show resolved Hide resolved
packages/palette_generator/lib/palette_generator.dart Outdated Show resolved Hide resolved
@mzdm mzdm marked this pull request as draft March 4, 2021 12:31
@mzdm mzdm marked this pull request as ready for review March 5, 2021 14:33
@mzdm mzdm requested a review from stuartmorgan March 5, 2021 14:34
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Everything I saw has been addressed; @gspencergoog should definitely do the final review here though since he knows the API.

@gspencergoog
Copy link
Contributor

Looking at it today.

@mzdm
Copy link
Contributor Author

mzdm commented Mar 8, 2021

after this revision all tests except one pass
(PaletteGenerator Filters work test)

dart:core                                                _AssertionError._throwNew
package:palette_generator/palette_generator.dart 74:16   new PaletteGenerator.fromColors
package:palette_generator/palette_generator.dart 124:29  PaletteGenerator.fromImage

'package:palette_generator/palette_generator.dart': Failed assertion: line 74 pos 16: 'paletteColors.isNotEmpty': is not true.

@gspencergoog
Copy link
Contributor

Sigh. Conceptually, it should be possible to enforce that paletteColors not be empty, but if the image contains all transparent pixels, or all the colors that exist in the image have been ignored by the caller, then it will indeed be empty. We could throw if one of those conditions happens, but I think it's better to just allow the list to be empty, which will result in a nullable dominantColor, which makes the most sense if there are no candidate colors. Thanks for trying that out anyhow.

@mzdm mzdm requested a review from gspencergoog March 10, 2021 12:38
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@gspencergoog gspencergoog merged commit c38b3d3 into flutter:master Mar 10, 2021
@mzdm mzdm deleted the nnbd-palette-gen branch March 10, 2021 18:36
stuartmorgan pushed a commit to stuartmorgan/packages that referenced this pull request Apr 30, 2021
austinstoker pushed a commit to austinstoker/packages that referenced this pull request Apr 29, 2022
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.

3 participants