Skip to content

Linter: spread collections phase 2 #57898

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

Closed
pq opened this issue Feb 1, 2019 · 4 comments
Closed

Linter: spread collections phase 2 #57898

pq opened this issue Feb 1, 2019 · 4 comments
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-lint-request linter-new-language-feature

Comments

@pq
Copy link
Member

pq commented Feb 1, 2019

Linter-specific issue corresponding to #35570 and tracking new lints to support spread collections as well as any spread collection enhanced behavior of existing ones.

New Rules

Proposal: prefer_spread_collections

(Corresponding roughly to the server assist: DartAssistKind.CONVERT_TO_SPREAD.)

Description: Use spread collections when possible.

Details: Collection literals are excellent when you want to create a new collection out of individual items. But, when existing items are already stored in another collection, spread collection syntax leads to simpler code.

BAD:

Widget build(BuildContext context) {
  return CupertinoPageScaffold(
    child: ListView(
      children: [
        Tab2Header(),
      ]..addAll(buildTab2Conversation()),
    ),
  );
}
var ints = [1, 2, 3];
print(['a']..addAll(ints.map((i) => i.toString()))..addAll(['c']));
var things;
var l = ['a']..addAll(things ?? const []);

GOOD:

Widget build(BuildContext context) {
  return CupertinoPageScaffold(
    child: ListView(
      children: [
        Tab2Header(),
        ...buildTab2Conversation(),
      ],
    ),
  );
}
var ints = [1, 2, 3];
print(['a', ...ints.map((i) => i.toString()), 'c');
var things;
var l = ['a', ...?things];

References

Language tracking issue: dart-lang/language#164
Feature specification: https://github.com/dart-lang/language/blob/master/accepted/future-releases/spread-collections/feature-specification.md
Implementation plan: https://github.com/dart-lang/language/blob/master/accepted/future-releases/spread-collections/implementation-plan.md

@pq
Copy link
Member Author

pq commented Mar 12, 2019

📟 @munificent, @kwalrath, @bwilkerson: input on word-smithing for the proposed prefer_spread_collections? (It's err, largely cough derived from the excellent feature spec.)

And say, is there an Effective Dart section in the works?

@bwilkerson
Copy link
Member

I'd remove the /*caret*/ marker from the third "bad" example.

The second "good" example should be

print(['a', ...ints.map((i) => i.toString()), ...['c']);

or, better yet

print(['a', ...ints.map((i) => i.toString()), 'c');

(Which suggests that we might want a separate rule to lint if the spread operator is applied to a literal collection, given that the elements of the operand could simply be inlined.)

Personally, I think the docs (in general, not just this one) would be better if we moved away from the nomenclature in the style guide. I think the style guide works because it's intentionally being prescriptive, but the lint rule docs should be descriptive, explaining what the rule will do. So, minimally, I'd drop the "Do".

I'd also not spend time explaining why the rule is a good idea when we can provide a pointer back to the style guide, such as "Derived from Effective Dart: [Do use spread collections when possible.](...)". It would also be helpful to have a link to the documentation for spread elements.

So, I'd probably write a description that explains that the rule finds places where a spread element could be used in place of addAll. (Though we might want to think about whether there are other cases the rule should flag.)

@pq
Copy link
Member Author

pq commented Mar 12, 2019

Thanks Brian!

I'd remove the /caret/ marker from the third "bad" example.

Fixed. (Whoops!)

The second "good" example should be...

Updated.

Personally, I think the docs (in general, not just this one) would be better if we moved away from the nomenclature in the style guide.

Yeah. I tend to agree. Ideally we could just update all of the descriptions this way though I guess technically this might be considered breaking (if folks are post-processing output)...

It would also be helpful to have a link to the documentation for spread elements.

Which docs? The spec? Or an as-yet-to-be written guide?

So, I'd probably write a description that explains that the rule finds places where a spread element could be used in place of addAll. (Though we might want to think about whether there are other cases the rule should flag.)

👍

@kwalrath: is this anything you'd have cycles to help with? Or if there's a write-up being prepared elsewhere can you point me to a place to crib notes (and ultimately link back?)

@pq pq added linter-lint-request area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). labels Mar 12, 2019
@bwilkerson
Copy link
Member

It would also be helpful to have a link to the documentation for spread elements.

Which docs? The spec? Or an as-yet-to-be written guide?

I'm assuming we have user-readable documentation about the language and that it will be updated to include the new features.

@pq pq closed this as completed Apr 29, 2019
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-lint-request linter-new-language-feature
Projects
None yet
Development

No branches or pull requests

3 participants