Add factory constructors for shorthands that would collide #165597
Conversation
There was a problem hiding this comment.
LGTM 👍
The shorthands feature is not planning to exhaustively check subtypes.
I think this is a key point of this feature for our users to understand. Is there anything we can do to help communicate that? I guess there's nowhere to document that in the Flutter docs, maybe just the Dart docs and feature announcement.
I'm thinking hard about whether the additional complexity of having these new overlapping constructors is offset by the simplicity of enum shorthand. I think the answer is yes it's worth it, but maybe it's something we should keep an eye on.
I'm also thinking about whether this pattern will result in us and our users needing to create lots of additional overlapping constructors in the future. I think that seems unlikely, and this PR is probably the worst case?
| assert( | ||
| (left == null && right == null) || (start == null && end == null), | ||
| 'The left and right values cannot be used in conjunction with start and end.', | ||
| ); |
There was a problem hiding this comment.
If everything is null, it will create a BorderRadius.horizontal with both parameters set to Radius.zero. I think that's what we want, but just wanted to call that out to double check.
| /// Utilizing the `left` and `right` properties will return a [BorderRadius], | ||
| /// while `start` and `end` will yield a [BorderRadiusDirectional]. These | ||
| /// properties cannot be used interchangeably. |
There was a problem hiding this comment.
I think it's good that you're calling out which subclasses will be created here in the docs, so that users are less confused that there are multiple ways to achieve the same thing 👍
There was a problem hiding this comment.
Whenever a factory constructor merely redirects to a constructor of a subtype and the signatures of the subtype constructor and redirecting constructors match, they can be written using the redirecting syntax. Also, if the subtype constructor is const, the redirecting factory constructor in the base class can also be const. This is the only case where a factory constructor is allowed to be const (const factory). Example:
abstract class Base {
const Base();
const factory Base.derived(int a, int b) = Derived;
factory Base.derivedNonConst(int a, int b) = Derived.nonConst;
}
class Derived extends Base { // or implements
const Derived(int a, b);
Derived.nonConst(int a, int b);
}I've added comments where this could be applied.
| /// Creates a [Border]. | ||
| /// | ||
| /// All the sides of the border default to [BorderSide.none]. | ||
| factory BoxBorder.fromLTRB({ |
There was a problem hiding this comment.
As per the existing conventions used in other places, fromLTRB and fromSTEB are usually used with positional parameters. Maybe BoxBorder.only would better maintain parity with the existing types like EdgeInsets.only. This can effectively be:
const factory BoxBorder.only(
BorderSide top,
BorderSide right,
BorderSide bottom,
BorderSide left,
}) => Border;
)There was a problem hiding this comment.
This is neat, but the goal is not to introduce new API that aligns with other classes, instead focusing on the existing API class (and its subclasses) making them compatible with the shorthands feature.
| /// the trailing edge. They are resolved during [paint]. | ||
| /// | ||
| /// All the sides of the border default to [BorderSide.none]. | ||
| factory BoxBorder.fromSTEB({ |
There was a problem hiding this comment.
As per the existing conventions used in other places, fromLTRB and fromSTEB are usually used with positional parameters. Maybe BoxBorder.directional would better maintain parity with the existing types like EdgeInsets.directional. We could still have another fromSTEB and fromLTRB constructors with positional parameters instead of named, but these do not currently exist in the subclasses. This can effectively be:
const factory BoxBorder.directional({
BorderSide top,
BorderSide start,
BorderSide end,
BorderSide bottom,
}) = BorderDirectional;
Yeah, this will be something for the Dart team to share and document when shorthands rolls out.
I totally agree about your concern. I don't think cases like this will be common though. These were identified by the Dart team's analysis for the feature as like - oh man these would be the most impactful based on usage - but they have this quirk of duped named constructors in the implementing classes. Upon reflection we thought, it would have been simpler to consolidate the classes since they are so similar. But just like shorthands would be really impactful for these classes, breaking them to change the architecture would also be really impactful (in the wrong way). I agree we should keep an eye on it, but I have thought of these as outliers in general. |
|
Thank you @amal-stack for checking me on these! Really appreciate the nice clean up. :) |
72342a4 to
7a53747
Compare
This adds convenience members to `AlignmentGeometry` in preparation for
Dart shorthands.
Before:
```dart
Container(
alignment: Alignment.topLeft,
child: Text('Hello world'),
),
```
After (with Dart shorthands):
```dart
Container(
alignment: .topLeft,
child: Text('Hello world'),
),
```
Prior art: #165597
## Pre-launch Checklist
- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel
on [Discord].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…65597) EdgeInsets and EdgeInsetsDirectional are subtypes of EdgeInsetsGeometry. The both have only, all, and symmetric constructors, which creates collisions in a world where Dart support shorthand notation such as: ```dart Padding( padding: .only(bottom: 10.0), ), ``` The same goes for all these classes: - AlignmentGeometry - Alignment - AlignmentDirectional - BorderRadiusGeometry - BorderRadius - BorderRadiusDirectional - BoxBorder - Border - BorderDirectional - EdgeInsetsGeometry - EdgeInsets - EdgeInsetsDirectional The shorthands feature is not planning to exhaustively check subtypes. However, these classes were identified as the most impactful for the shorthands feature (❗ ). Adding these factories in the super classes enables future shorthand support. Prior in flutter#159703 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
EdgeInsets and EdgeInsetsDirectional are subtypes of EdgeInsetsGeometry.
The both have only, all, and symmetric constructors, which creates collisions in a world where Dart support shorthand notation such as:
The same goes for all these classes:
The shorthands feature is not planning to exhaustively check subtypes. However, these classes were identified as the most impactful for the shorthands feature (❗ ). Adding these factories in the super classes enables future shorthand support.
Prior in #159703
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.