Dev-exp team packages lints

[FMorschel]

This issue tracks lint rules the developer-experience team may wish to enable in their packages (more info at this comment).

Old description

While working on https://dart-review.googlesource.com/c/sdk/+/401865 I noticed that pkg/dartdev/test/commands/fix_test.dart is not sorted as are the analysis_server files. Maybe some other packages are also not enforcing the same standards? Opening this issue to track this conversation.

We could also take a look at the analysis_option.yamls for them so we are more consistent as I've also noticed in other CLs that there are differences between the analysis_server and linter packages.

CC some of the people I've either talked about it or I think may have an opinion on the matter @bwilkerson @scheglov @srawlins @pq.

[dart-github-bot]

Summary: Inconsistent code sorting and linting rules across Dart SDK packages were observed. This issue tracks investigation and standardization efforts.

[lrhn]

There are no official rules for sorting code in the platform libraries or packages.
As far as I know, only the analyzer team has chosen to sort members. Since there are no rules, that's not an incorrect ordering.

The two traditional perspectives here are:

• Code authors should not need to make a choice, there is only one correct ordering and it's enforced by tools.
• Alphabetical ordering is meaningless. It's usually better for readability to sort semantically, with related functions being close.

(I'm personally strongly in the second group.)

[FMorschel]

On personal small projects, I do prefer semantics for sorting, but I think it gets a bit complicated when you start having lots of small methods that are used in more than one place or that depend on each other in a "loop", or any other difficult-to-place anywhere reason.

(Take a look at pkg\analysis_server\lib\src\services\correction\dart\import_library.dart, where the producers getter needs 6 different methods that internally need others in common, it gets complicated to sort by semantics here.)

In big projects like this, I agree we should have some form of standard (hence this issue) so everyone knows what to expect.

But that was simply what caught my eye recently. I think the lints are even more impactful of a change since they'll help enforce what standards we'd like to follow (and avoid some meaningless reviews).

I won't remember which CL it was, but I had to add one new propriety to a class or something like that, and there was a missing empty line between it and the class constructor (I'm aware there is no lint for this currently but it is simply an example), everything else on the CL was fine, but I had to apply a change and wait for two new reviews after that.

If these simple things could be enforced by a lint, then some similar cases could be avoided.

See the difference

analysis_server

linter:
  rules:
    - analyzer_use_new_elements
    - avoid_redundant_argument_values
    - flutter_style_todos
    - library_annotations
    - prefer_single_quotes
    - unawaited_futures
    - unnecessary_breaks
    - unnecessary_final
    - unnecessary_library_directive
    - unnecessary_parenthesis
    - unreachable_from_main

linter

linter:
  rules:
    - always_put_required_named_parameters_first
    - analyzer_use_new_elements
    - avoid_annotating_with_dynamic
    - avoid_catches_without_on_clauses
    - avoid_catching_errors
    - avoid_dynamic_calls
    - avoid_positional_boolean_parameters
    - avoid_print
    - avoid_redundant_argument_values
    - avoid_returning_this
    - avoid_setters_without_getters
    - avoid_slow_async_io
    - avoid_unused_constructor_parameters
    - cancel_subscriptions
    - cast_nullable_to_non_nullable
    - comment_references
    - directives_ordering
    - discarded_futures
    - flutter_style_todos
    - join_return_with_assignment
    - library_annotations
    - literal_only_boolean_expressions
    - no_adjacent_strings_in_list
    - noop_primitive_operations
    - only_throw_errors
    - parameter_assignments
    - prefer_asserts_in_initializer_lists
    - prefer_const_constructors_in_immutables
    - prefer_constructors_over_static_methods
    - prefer_expression_function_bodies
    - prefer_foreach
    - prefer_null_aware_method_calls
    - prefer_relative_imports
    - prefer_single_quotes
    - sort_pub_dependencies
    - test_types_in_equals
    - throw_in_finally
    - unawaited_futures # pedantic
    - unnecessary_breaks
    - unnecessary_final
    - unnecessary_lambdas
    - unnecessary_library_directive
    - unnecessary_null_checks
    - unnecessary_parenthesis
    - unnecessary_statements
    - unreachable_from_main
    - use_if_null_to_convert_nulls_to_bools
    - use_late_for_private_fields_and_variables
    - use_setters_to_change_properties
    - use_string_buffers
    - use_to_and_as_if_applicable

I'm well aware that either is going to be a big change and probably won't happen anytime soon, this was simply to point this out so we can have this in mind and discuss it.

---

P.S.: Small side note since this can help to reduce meaningless reviews. Another place where we could have some thoughts is the Copybara and GitHub PRs relation, see dart-lang/dart_ci#166.

[FMorschel]

One lint I also see (at least in analyzer-related pkgs) that should be added is always_put_control_body_on_new_line since that is enforced by reviewers but is not there.

[lrhn]

That lint is definitely not consistently being enforced.
I know I write plenty of one-line if-return and if-throw statements.

If a team likes it, they can obviously add it to their packages.

[FMorschel]

What about comment_references (CC @srawlins this would be great with Doc Imports)? I just found:

https://github.com/dart-lang/sdk/tree/dbf60082974a075c4a80290189f3d854f533fdbd/pkg/analyzer_plugin/lib/protocol/protocol_common.dart#L3506-L3519

Which mentions FileEdit above two methods but on VSCode there is no reference to it.

[srawlins]

I've been cleaning up comment_references in the analyzer package, analysis_server, and devtools repo. And I haven't found anything that couldn't be fixed (previously that rule had false positives). I'd like to enable it.

[srawlins]

It would be good to narrow this issue down to: lint rules that the developer-experience team may wish to enable in their packages. Lasse addressed the sorting topic above. And each team is free to enable or disable lint rules as fits their culture, so I think it would be too open-ended to try to get all SDK packages to enable this or that lint rule. As far as organization goes, the SDK git repo is often just the most convenient place to store the source for this or that package. It doesn't indicate any singular coding style should be used across the entire git repo.

[FMorschel]

I'm fine with that. Once I'm back (on my phone now) I'll update the isssue description and title to reflect this. Thanks for the feedback everyone!