The order of 'Encapsulate field' loses dartdocs.

[gspencergoog]

Currently when using Encapsulate field, the order of the generated values is private var, getter, setter.

It would make more sense to have the generated order be: getter, private var, then setter, instead of having the private var come first: if you had dartdoc documentation on the original var, then it will get lost when encapsulating, since dartdoc will now think the /// comment is on the private field.

For example, starting with:

  /// Counts the foos in a bar.
  int foo;

and you run "Encapsulate field" on foo, then you get:

  /// Counts the foos in a bar.
  int _foo;

  int get foo => _foo;

  set foo(int foo) {
    _foo = foo;
  }

Which means that dartdoc loses the original documentation, because it now sees that as attached to the private var, and not the getter, and so it needs to be moved manually.

It should generate it like this to retain the docs:

  /// Counts the foos in a bar.
  int get foo => _foo;

  int _foo;

  set foo(int foo) {
    _foo = foo;
  }

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/52267

[gspencergoog]

That change looks like I'll end up with this instead:

  int _foo;

  /// Counts the foos in a bar.
  int get foo => _foo;

  set foo(int foo) {
    _foo = foo;
  }

That's not horrible (at least the docs are preserved), but real-world usage looks more like this, which makes it seem like the private var isn't really part of the "grouping" that goes together when you read the code, which is why I mentioned having the private after the getter instead:

  ParentData _parentData;

  /// Data for use by the parent render object.
  ///
  /// The parent data is used by the render object that lays out this object
  /// (typically this object's parent in the render tree) to store information
  /// relevant to itself and to any other nodes who happen to know exactly what
  /// the data means. The parent data is opaque to the child.
  ///
  ///  * The parent data field must not be directly set, except by calling
  ///    [setupParentData] on the parent node.
  ///  * The parent data can be set before the child is added to the parent, by
  ///    calling [setupParentData] on the future parent node.
  ///  * The conventions for using the parent data depend on the layout protocol
  ///    used between the parent and child. For example, in box layout, the
  ///    parent data is completely opaque but in sector layout the child is
  ///    permitted to read some fields of the parent data.
  ParentData get parentData => _parentData;

  set parentData(ParentData parentData) {
    _parentData = parentData;
  }

[scheglov]

This is a slightly different problem - we put getter/setter right after the field.
Ideally we probably should put them inside other getters/setters.
The order "getter, field, setter" does not look particularly good to me.

Alternatively Code | Sort members in Dart file in IntelliJ will do this automatically for all members. But it does much more.

[gspencergoog]

Maybe the field could come last (getter, setter, field)? That would also keep them grouped.

[scheglov]

Oh, it seems that we have quite different ideas about the order of members in classes. The order we use in Analyzer is: constants, static fields, static accessors, instance fields, constructors, instance getters, instance methods, static methods. Inside each group we sort members alphabetically. I have never before seen grouping fields and accessors together in Dart code. Which projects do this?

[gspencergoog]

Flutter does. From our style guide: "The methods, properties, and other members of a class should be in an order that will help readers understand how the class works." "...Mutable properties, each in the order getter, private field, setter, without newlines separating them."

[scheglov]

Ah, I see. Of course Flutter has a bit different code style.

Hm... Honestly I'm not sure what to do here.
This is not what most of the other code does.

We could go the way of making a lot of code style settings, somewhere in analysis_options.yaml - order of declarations, adding type for extracted locals, add or omit new keyword. Support for settings costs something, but if it is important, we could do this. @devoncarew @bwilkerson

[gspencergoog]

Well, it sure would be useful to have these actions be configurable. If you had those options, then you could also do something along the line of "Sort class members", but have it apply the ordering in the options instead of sorting alphabetically.