Skip to content

Lints to be considered for score.yaml or recommend.yaml from Flutter #58349

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
goderbauer opened this issue Mar 16, 2021 · 4 comments
Closed
Assignees
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead.

Comments

@goderbauer
Copy link
Contributor

goderbauer commented Mar 16, 2021

The following lints are no-ops for non-Flutter code and adding them to the general lint set shouldn't cause any harm. They help developers craft clean UI code without unnecessary cruft while honoring the widget lifecycle contract and encourage good widget design practices.

  • avoid_unnecessary_containers
  • avoid_web_libraries_in_flutter
  • no_logic_in_create_state
  • sized_box_for_whitespace
  • use_full_hex_values_for_flutter_colors
  • use_key_in_widget_constructors

The following lints also affect non-Flutter code, but I hope they can be added to the general lint set as well to have alignment between regular Dart and Flutter code (if not, they should go into a Flutter-specific lint set). Encouraging the use of const Widgets is beneficial for performance.

  • prefer_const_constructors
  • prefer_const_constructors_in_immutables
  • prefer_const_declarations
  • prefer_const_literals_to_create_immutables
  • avoid_type_to_string (currently broken: Rule avoid_type_to_string not working #58377) - avoids hard to debug differences between debug and release mode
  • no_runtimeType_toString (subset of avoid_type_to_string? If yes, not needed.) - discourages performance pitfall
  • prefer_if_elements_to_conditional_expressions - cleaner code when generating children lists

The following lint is not appropriate for non-Flutter apps and should go into a Flutter-specific lint set:

  • avoid_print - avoids polluting production logs

@mit-mit and/or @munificent to figure out if the first two lists in this bug are OK to add to the general lint set.

/cc @Hixie @pq @devoncarew

@mit-mit
Copy link
Member

mit-mit commented Mar 17, 2021

Bob, OK that I assign this to you?

@davidmorgan
Copy link
Contributor

Thanks Michael!

I'm in favour of adding lints that only fire on Flutter code to the general set, it's simpler. Without being familiar with Flutter I can't evaluate the lints themselves; use_full_hex_values_for_flutter_colors is the only one of those we enforced in google3 so far.

We've generally followed the idea that lints should have no false positives to make the recommended list. So if any of those have the expectation that users should sometimes add an // ignore comment, then we might want to dig into that and try to improve it.

Re: the general list, we decided against all those in google3, more details:

avoid_type_to_string and no_runtimeType_toString -- I think the first one includes the second? In which case we should only need to enforce one of the two :)

There is a need for Type.toString in some core libraries, e.g. it's the only way to map from type with generics to raw type. It might also make sense in tests. So we thought this was too strict as-is.

The prefer_const lints have downsides for general code, they make it more verbose and declaring something as const means you can't evolve it to non-const later.

prefer_if_elements_to_conditional_expressions looks over-broad, the example given in the lint docs is with a null on the RHS of the ?:, where the if is clearly better. But when we checked in google3 it also fires for cases where there are values on both sides, and writing if/else is longer and not obviously better. Perhaps the scope could be narrowed?

Finally:

avoid_print is something we let teams opt into but do not apply generally.

@goderbauer
Copy link
Contributor Author

avoid_type_to_string and no_runtimeType_toString -- I think the first one includes the second? In which case we should only need to enforce one of the two :)

True, no_runtimeType_toString appears to be a subset of avoid_type_to_string, at least according to the docs. In practice, avoid_type_to_string appears to be broken: #58377. If that gets fixed, we'd only need to include the stronger lint.

@goderbauer
Copy link
Contributor Author

Closing this since the lint packages with most of these lints have been released.

@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants