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.

Sign up for GitHub

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

Jump to bottom

proposal: move_color_to_decoration #3063

Open
5 tasks done
minhqdao opened this issue Nov 16, 2021 · 6 comments · May be fixed by #3064
Open
5 tasks done

proposal: move_color_to_decoration #3063

minhqdao opened this issue Nov 16, 2021 · 6 comments · May be fixed by #3064
Labels
lint-proposal P3 A lower priority bug or feature request status-pending type-enhancement A request for a change that isn't a bug

Comments

@minhqdao
Copy link
Contributor

minhqdao commented Nov 16, 2021 •
edited

move_color_to_decoration

Description

Don't provide Container with both non-null color and decoration. Place color inside decoration instead.

Details

When a Container is provided with both color and decoration (both non-null), an error is thrown at runtime, and there is currently nothing that warns you about that beforehand. This rule prevents you from running into the error by prompting to move color into decoration.

Kind

Guard against errors.

Good Examples

Widget buildArea() {
  return Container(
    decoration: BoxDecoration(
      border: Border.all(color: Colors.white),
      color: Colors.black,
    ),
  );
}

Bad Examples

Widget buildArea() {
  return Container(
    color: Colors.black,
    decoration: BoxDecoration(
      border: Border.all(color: Colors.white),
    ),
  );
}

Discussion

Those who already ran into the error will know, but those who haven't will certainly do, and that's simply annoying. No one wants to dig through unexpected error messages, even if this one is rather easy to identify.

Discussion checklist

  • List any existing rules this proposal complements, overlaps or conflict with.
    Slightly complementing: proposal: use_decorated_box #3060 use_decorated_box #3061
  • List any relevant issues (reported here, the SDK Tracker, or elsewhere).
    Not found.
  • If there's any prior art (e.g., in other linters), please add references here.
    None.
  • If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn’t any corresponding advice, should there be?)
  • If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.
@minhqdao minhqdao linked a pull request Nov 16, 2021 that will close this issue
Open
@pq
Copy link
Member

pq commented Nov 16, 2021

/fyi @Hixie @goderbauer @HansMuller

@goderbauer
Copy link
Contributor

This seems useful to inform people of an illegal configuration at code write time instead of compile time.

@HansMuller
Copy link

I agree, if we can flag this problem early it's a win for developers.

@pq
Copy link
Member

pq commented Nov 16, 2021 •
edited

This is terrific and addresses a problem that really trips folks up. I looked at this a while ago and recall there are at least a handful of other Flutter classes w/ similar constraints. Doing a quick scan now I see similar patterns in at least: CupertinoSearchTextField, Ink, TextStyle, and AnimatedContainer. Given that, it'd be great to revisit a more general and robust solution.

Using an annotation, for example, we could capture this constraint at the API level.

Details

EDIT: removed annotation proposal in favor of centralizing that discussion in dart-lang/sdk#47712

/fyi @bwilkerson @InMatrix @leafpetersen @munificent who were in on the original conversation about annotating exclusive params vs. a language solution like overloading.

/fyi also @natebosch @jakemac53 @lrhn @eernstg for more language team perspective

@bwilkerson
Copy link
Member

In terms of using an annotation for this, I think the right next step is to create a new issue (in the sdk repo) to discuss the kinds of information we'd need to capture for the use cases we know about, and then the right set of annotations with which to capture that information. It might well be that OnlyOneOf would meet all of the known needs, but I'd like to know that before we make a decision.

@pq pq mentioned this issue Nov 16, 2021
Open
@pq
Copy link
Member

pq commented Nov 16, 2021

@bwilkerson, agreed.

I created dart-lang/sdk#47712 and updated my proposal above to link out to it.

@srawlins srawlins added the P3 A lower priority bug or feature request label Oct 4, 2022
@srawlins srawlins mentioned this issue Feb 8, 2024
Open
5 tasks
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
lint-proposal P3 A lower priority bug or feature request status-pending type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement move_color_to_decoration minhqdao/linter
6 participants
@pq @srawlins @goderbauer @HansMuller @bwilkerson @minhqdao