Some SDK pkgs not sorted and different lints

[FMorschel]

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.