Add @protected to public State method overrides
#157313
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
bot
added
a: text input
This comment was marked as resolved.
This comment was marked as resolved.
nate-thegrate
force-pushed
the
always-use-protection
branch
from
876bbd6 to
e4f729f
Compare
|
I imagine the team's overall preference would be to move forward with this PR, but #157874 is also a fun option! |
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
lgtm with the caveat that I don't have the domain knowledge to understand the implications of the changes in this PR.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I've requested Google tests on this PR. That will be a good indicator on whether or not people are referencing these methods in their apps. It's encouraging that the customer tests are already passing at least.
This all sounds like a good idea to me, but I'm worried it's risky, so ideally we can get some other people to look at this too (maybe @goderbauer when he's back?). I wonder why these methods weren't marked @protected in the first place, just an oversight? I see that they are @protected in their base class, e.g. State.initState is protected.
Your design doc and #157874 make a good case for the extension solution, and we've been up for making exceptions to the styleguide lately, but doing that all over the framework would be a huge departure from the intent of the styleguide. The solution in this PR is probably better.
tl;dr: LGTM but we should get more people to look at this.
|
Thanks very much @justinmc! Hope you don't mind if I argue a couple of points :)
If, for some reason, someone has
I don't believe the other PR violates any style guidelines, since "extension methods" and "extension types" are separate concepts.
But as far as "the intent of the styleguide", I can see how just having the public class without the extension type wrapper would be more readable for people debugging their own projects 👍 |
This comment was marked as resolved.
This comment was marked as resolved.
|
Looks like the Google tests are passing which is really encouraging! I see your point about Maybe we can figure out a path forward for this in the triage meeting next week. I could definitely be convinced either way. And I do think we should fix this problem one way or another. Also it would be nice if we could update the styleguide to explicitly state what our policy is on |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 . Thanks for going through all of the back and forth while deciding whether to take this approach or the other one.
I assume your new lint passes on this branch?
I admit I haven't added a custom lint rule before so I'm not super familiar with that code, maybe someone who knows more should take a quick look at that before merging if possible.
nate-thegrate
force-pushed
the
always-use-protection
branch
from
5a0aad5 to
16bda7e
Compare
nate-thegrate
left a comment
nate-thegrate
left a comment
There was a problem hiding this comment.
Just caching a single DartType was the right call 👍
| // TODO(nate-thegrate): remove this file if @protected changes, or add a test if it doesn't. | ||
| // https://github.com/dart-lang/sdk/issues/57094 |
There was a problem hiding this comment.
I think the conversation there is pretty interesting:
| /// Holds the `State` class [DartType]. | ||
| static DartType? stateType; |
| // If @protected State methods are added or removed, the analysis rule should be | ||
| // updated accordingly (dev/bots/custom_rules/protect_public_state_subtypes.dart) |
There was a problem hiding this comment.
(appended this to the State class declaration)
nate-thegrate
force-pushed
the
always-use-protection
branch
from
16bda7e to
157dfb7
Compare
nate-thegrate
added
the
autosubmit
This comment was marked as resolved.
This comment was marked as resolved.
auto-submit
bot
removed
the
autosubmit
This comment was marked as resolved.
This comment was marked as resolved.
nate-thegrate
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
flutter/flutter@b3818f6...8536b96 2024-11-19 [email protected] Terminate `flutter test` when no longer needed in integration test. (flutter/flutter#159117) 2024-11-19 [email protected] Terminate the test device if the `flutter` tool is signal-killed. (flutter/flutter#159115) 2024-11-19 [email protected] Roll Packages from c1eabf5 to fc4adc7 (10 revisions) (flutter/flutter#159143) 2024-11-19 [email protected] Deflake api 35 emulator tests by updating emulator definitions version to latest available from chrome infra (flutter/flutter#158017) 2024-11-19 [email protected] Roll Flutter Engine from b6723e33b858 to cff1e751f853 (1 revision) (flutter/flutter#159141) 2024-11-19 [email protected] Fix InkWell overlayColor resolution ignores selected state (flutter/flutter#159072) 2024-11-19 [email protected] Roll Flutter Engine from 983b7b85d122 to b6723e33b858 (3 revisions) (flutter/flutter#159134) 2024-11-19 [email protected] Roll Flutter Engine from c1b0e18a70b3 to 983b7b85d122 (2 revisions) (flutter/flutter#159124) 2024-11-19 [email protected] Roll Flutter Engine from 878f593802e1 to c1b0e18a70b3 (13 revisions) (flutter/flutter#159118) 2024-11-19 [email protected] [SwiftPM] Move where the migration checks feature flags (flutter/flutter#159110) 2024-11-18 [email protected] Plumbs `scrollBehavior` into `SelectableText` so that the scrollbar may be hidden (flutter/flutter#158887) 2024-11-18 [email protected] Add a tag and assert some state in FlutterTestDriver tests. (flutter/flutter#159099) 2024-11-18 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.6.0 to 5.0.2 in the all-github-actions group (flutter/flutter#159104) 2024-11-18 [email protected] Add `@protected` to public `State` method overrides (flutter/flutter#157313) 2024-11-18 [email protected] Roll Flutter Engine from 9686de154114 to 878f593802e1 (5 revisions) (flutter/flutter#159097) 2024-11-18 [email protected] Fix flaky failure related to core_device_list.json not being found (flutter/flutter#158946) 2024-11-18 [email protected] Prettier merge_queue.md (flutter/flutter#158969) 2024-11-18 [email protected] Define and use `flutterBin` consistently across `integration.shard`. (flutter/flutter#159007) 2024-11-18 [email protected] No longer download `android-x86-jit-release`. (flutter/flutter#159011) 2024-11-18 [email protected] Roll Flutter Engine from e1f4e7d9bfc4 to 9686de154114 (1 revision) (flutter/flutter#159082) 2024-11-18 [email protected] Roll Flutter Engine from f365c9f5dce3 to e1f4e7d9bfc4 (4 revisions) (flutter/flutter#159078) 2024-11-18 [email protected] Roll Packages from b164be3 to c1eabf5 (6 revisions) (flutter/flutter#159077) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
nate-thegrate
added
the
autosubmit
4 participants
I don't want to see these methods in my autofill suggestions.