[file_selector] Convert XTypeGroup to const by eugerossetto · Pull Request #6476 · flutter/plugins

eugerossetto · 2022-09-23T17:44:31Z

Within the file_selector_platform_interface, XTypeGroup constructor was converted to const, meaning that from now on, this object needs to be constructed using const.

Issue:
#111906 [file_selector] XTypeGroup should be immutable

Related PR:
#6463 Annotate all creation of XTypeGroup with // ignore: prefer_const_contructor

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

stuartmorgan-g

LGTM with nits. @ditman can you do the secondary review here?

.../file_selector/file_selector_platform_interface/lib/src/types/x_type_group/x_type_group.dart

stuartmorgan-g · 2022-09-23T18:38:23Z

.../file_selector/file_selector_platform_interface/lib/src/types/x_type_group/x_type_group.dart


+  final List<String>? _extensions;
+
+  /// The extensions for this group


Nit: This is missing the period at the end. (If you could fix the other comments' missing periods while you are touching this that would be great.)

.../file_selector/file_selector_platform_interface/lib/src/types/x_type_group/x_type_group.dart

ditman

This looks good to me, however it breaks the current prefer_const_constructors (and will possibly be a breaking change for users of the package (depending on their build config)).

Maybe a major version is too hardcore for this change (but maybe make this 2.2.0?).

Also, we need to deal with the prefer_const_constructors lint. We could disable it so we can land this, and fix usages of XTypeGroup across the repo, or fix Everything Everywhere All At Once in this PR.

Warning: You are using these overridden dependencies:
! file_selector_platform_interface 2.1.1 from path ../../file_selector/file_selector_platform_interface
Running command: "dart analyze --fatal-infos" in /tmp/cirrus-ci-build/packages/image_picker/image_picker_windows
Analyzing image_picker_windows...

   info - lib/image_picker_windows.dart:122:9 - Prefer const with constant constructors. - prefer_const_constructors
   info - lib/image_picker_windows.dart:146:9 - Prefer const with constant constructors. - prefer_const_constructors
   info - lib/image_picker_windows.dart:162:9 - Prefer const with constant constructors. - prefer_const_constructors

3 issues found.


�[31mThe following packages had errors:�[0m
�[31m  file_selector/file_selector�[0m
�[31m  file_selector_ios�[0m
�[31m  file_selector_linux�[0m
�[31m  file_selector_macos�[0m
�[31m  file_selector_web�[0m
�[31m  file_selector_windows�[0m
�[31m  image_picker_windows�[0m

ditman

I've seen the PR that prepares the repo to land this. (I still think this might need a higher version number.)

stuartmorgan-g · 2022-09-23T19:31:15Z

Maybe a major version is too hardcore for this change (but maybe make this 2.2.0?).

We don't consider adding consts to be a semver breaking change, but you're right that a minor change is a better match in semver for this—it's an API change, but a compatible one (other than lints)—so 2.2.0 sounds good.

eugerossetto · 2022-09-23T20:16:40Z

Hi guys, thanks for the feedback, I applied all the requested changes, I hope this looks good for you.
If there's anything else you would like to add/change just let me know.

ditman · 2022-09-23T22:12:43Z

Hi guys, thanks for the feedback, I applied all the requested changes, I hope this looks good for you.
If there's anything else you would like to add/change just let me know.

This LGTM, but #6463 needs to land first so this can land without errors in CI.

stuartmorgan-g

LGTM with one nit.

You'll need to merge in main now so that analyze will pass.

stuartmorgan-g · 2022-09-26T18:27:52Z

packages/file_selector/file_selector_platform_interface/CHANGELOG.md

-## NEXT
+## 2.2.0

+* Makes XTypeGroup constant and changes the initialization of it.


The second part of this is confusing since it's only referring to tests. How about just

* Makes `XTypeGroup`'s constructor constant.

auto-submit · 2022-09-26T18:53:16Z

auto label is removed for flutter/plugins, pr: 6476, due to - The status or check suite legacy-version-analyze CHANNEL:2.10.5 has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite analyze CHANNEL:stable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite downgraded_analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite analyze CHANNEL:master has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite legacy-version-analyze CHANNEL:3.0.5 has failed. Please fix the issues identified (or deflake) before re-applying this label.

stuartmorgan-g · 2022-09-26T18:54:41Z

Looks like the other PR missed a few.

eugerossetto · 2022-09-26T19:58:08Z

Hi Stuart, the pipeline informs that image_picker_windows is having issues with the lint, do you want me to open another PR to fix that?
On the other hand, we see that on CHANNEL:stable, several linters are failing, but we added them on the just merged PR.
This differs from the errors reported on the CHANNEL:master pipeline, which actually does not report failures on the file_selector package, is there something that we missed?
Thanks!

stuartmorgan-g · 2022-09-26T20:08:51Z

On the other hand, we see that on CHANNEL:stable, several linters are failing, but we added them on the just merged PR. This differs from the errors reported on the CHANNEL:master pipeline, which actually does not report failures on the file_selector package, is there something that we missed? Thanks!

I don't follow the question; both runs that you linked to report the same set of failures. There's apparently another lint violation that you didn't suppress.

You should run dart ../plugins/script/tool/bin/flutter_plugin_tools.dart analyze over a checkout of the repo with this PR applied locally to make sure you've actually fixed all violations of lints that this will cause.

eugerossetto · 2022-09-27T12:53:51Z

Hello Stuart, we're creating a PR to fix the lint issues on the image_picker_windows and another one to fix the new lint issues related to prefer_const_literals_to_create_immutables. In addition, we want to ask you if we should use ignore_for_file instead of ignore in every line, because there are a lot and some of them are just below each other, making them very hard to read.
Thanks!

stuartmorgan-g · 2022-09-27T14:06:34Z

It's okay to do a file-level ignore since this should be very short-lived.

…s#6476)

eugerossetto requested a review from stuartmorgan-g as a code owner September 23, 2022 17:44

github-actions bot added the p: file_selector label Sep 23, 2022

eugerossetto force-pushed the 111906-xtypegrup-constructor-change branch from f2abb61 to 2fbaeb2 Compare September 23, 2022 17:51

stuartmorgan-g suggested changes Sep 23, 2022

View reviewed changes

stuartmorgan-g requested a review from ditman September 23, 2022 18:40

ditman suggested changes Sep 23, 2022

View reviewed changes

ditman self-requested a review September 23, 2022 19:02

ditman approved these changes Sep 23, 2022

View reviewed changes

stuartmorgan-g approved these changes Sep 26, 2022

View reviewed changes

eugerossetto force-pushed the 111906-xtypegrup-constructor-change branch from 7cd2fd0 to 12e9583 Compare September 26, 2022 18:47

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2022

This was referenced Sep 27, 2022

[file_selector] Annotate all parameters of XTypeGroup with // ignore: prefer_const_literals_to_create_immutables #6499

Merged

[image_picker_windows] Annotate XTypeGroup constructor with // ignore: prefer_const_constructors #6504

Merged

eugerossetto force-pushed the 111906-xtypegrup-constructor-change branch from 12e9583 to 4bffe59 Compare September 27, 2022 17:49

eugerossetto and others added 3 commits September 30, 2022 13:16

Make XTypeGroup constant and refactor every instance of it.

2b5ef5d

Update to version 2.2.0 and add missing periods.

edd2825

Fix merge conflicts in changelog.md file

4d1f556

baezfacundo force-pushed the 111906-xtypegrup-constructor-change branch from 4bffe59 to 4d1f556 Compare September 30, 2022 17:01

Fix merge issue on Updates imports for prefer_relative_imports.

569dc59

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 1, 2022

auto-submit bot merged commit 28c9c0d into flutter:main Oct 1, 2022

engine-flutter-autoroll mentioned this pull request Oct 3, 2022

Roll Plugins from dc5f1855cb62 to 28c9c0d034d9 (2 revisions) flutter/flutter#112802

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2022

28c9c0d03 [file_selector] Convert XTypeGroup to const (flutter/plugin…

d5e1fc7

…s#6476)

This was referenced Oct 6, 2022

[file_selector] Use const instead of final for XTypeGroup. #6542

Merged

[image_picker_windows] Use const instead of final for XTypeGroup. #6544

Merged

adpinola deleted the 111906-xtypegrup-constructor-change branch October 11, 2022 12:46

adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022

[file_selector] Convert XTypeGroup to const (flutter#6476)

14046fb

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[file_selector] Convert XTypeGroup to const (flutter#6476)

829adb8


		final List<String>? _extensions;

		/// The extensions for this group

Conversation

eugerossetto commented Sep 23, 2022

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Sep 23, 2022

Uh oh!

eugerossetto commented Sep 23, 2022

Uh oh!

ditman commented Sep 23, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Sep 26, 2022

Uh oh!

stuartmorgan-g commented Sep 26, 2022

Uh oh!

eugerossetto commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Sep 26, 2022

Uh oh!

eugerossetto commented Sep 27, 2022

Uh oh!

stuartmorgan-g commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ditman left a comment •

edited

Loading

eugerossetto commented Sep 26, 2022 •

edited

Loading