new lint: sort_child_properties_last

[pq]

Name TBD but the idea is to nudge people to sorting child/children properties last in their build methods to improve UI-as-Code feature readability.

@jacob314: could you add some more context? Maybe some concrete exemplars?

[a14n]

Close to #1129. Alternatively we could add a lint to ensure named arguments are provided in the same order as in the method declaration.

[jacob314]

We should do both. Inside package:flutter it would make sense to lint that named arguments that are Widget and Iterable<Widget> occur last in the list of arguments. From my perspective I don't care if named arguments are out of order as long as they are sorted so Widget and Iterable<Widget> arguments are last.

[pq]

@jacob314: could you add some real-world examples and help me fill in the motivation?

Spitballing a description:

Sort child properties last in widget instance creations.  This improves
readability and generally plays nicest with auto-formatting and UI-as-code 
language constructs.

[jacob314]

Here is an example of how violating this pattern hurts the utility of the UI as Code Guides.

Bad property sorting.

Notice it is hard to see what widget the onPressed, and mainAxisAlignment properties are associated with as they occur after the lines are drawn. Properties in the correct order appear clearly associated with the constructor call and separated from the children.

With properties listed before child widgets:

Background info: In the inspector tree view, and toStringDeep text dumps of Widget trees, children are always listed last. This lint brings lints for how code authors write code in alignment. UI as code widget tree visualization makes widget build methods look more similar to inspector and toStringDeep dumps for trees so having alignment now has more value than previously.

Most constructor definitions and constructor calls in package:flutter already match this lint so this lint is for the most part matching the facts on the ground and enforcing consistency instead of changing how most code in the framework is written.

Notice that child and children are listed last in these constructor definitions:

  Row({
    Key key,
    MainAxisAlignment mainAxisAlignment = MainAxisAlignment.start,
    MainAxisSize mainAxisSize = MainAxisSize.max,
    CrossAxisAlignment crossAxisAlignment = CrossAxisAlignment.center,
    TextDirection textDirection,
    VerticalDirection verticalDirection = VerticalDirection.down,
    TextBaseline textBaseline,
    List<Widget> children = const <Widget>[],
  })

  ConstrainedBox({
    Key key,
    @required this.constraints,
    Widget child,
  })  : assert(constraints != null),
        assert(constraints.debugAssertIsValid()),
        super(key: key, child: child);

[a14n]

Perhaps the lint should apply only if there's only one argument that is Widget or Iterable.

For instance, putting Widgets at the end of argument list will dissociate the widget from its alignment for the following sample:

MyWidget({
  CrossAxisAlignment headerAlignment = CrossAxisAlignment.center,
  Widget header,
  CrossAxisAlignment contentAlignment = CrossAxisAlignment.start,
  Widget content,
})

[pq]

Good example @a14n.

TBH: my initial instinct is to be more narrow still and limit it to child and children properties to avoid possible (and potentially confusing / annoying) false positives.

[pq]

@jacob314: the first cut has landed. Feel free to chime in here or separately if you'd like to see any tweaks to description and/or content. 👍

[a14n]

I'm trying to apply this new lint to flutter code base and I find some case less readable IMH (especially when callback is used). For instance:

//-------------- before
RaisedButton(
  child: const Text('CONFIRMATION'),
  onPressed: () {
    showTimePicker(
      context: context,
      initialTime: _selectedTime,
    )
    .then<void>((TimeOfDay value) {
      if (value != null && value != _selectedTime) {
        _selectedTime = value;
        _scaffoldKey.currentState.showSnackBar(SnackBar(
          content: Text('You selected: ${value.format(context)}'),
        ));
      }
    });
  },

//-------------- after
RaisedButton(
  onPressed: () {
    showTimePicker(
      context: context,
      initialTime: _selectedTime,
    )
    .then<void>((TimeOfDay value) {
      if (value != null && value != _selectedTime) {
        _selectedTime = value;
        _scaffoldKey.currentState.showSnackBar(SnackBar(
          content: Text('You selected: ${value.format(context)}'),
        ));
      }
    });
  },
  child: const Text('CONFIRMATION'),
),

WDYT?

[pq]

Thanks for doing this!

My understanding is that this formatting really shines w/ the method guides being added to IDEs... I'd be curious though if there are cases where we don't want to recommend it?

/cc @jacob314

[a14n]

@jacob314 @hixie regarding my previous comment, does the result look good too you?

[jacob314]

Here are screenshots of that example with child at the start of the list and at the end of the list. In this case the child widget tree is trivial so the difference isn't huge. This convention matters more when the child widget tree is larger so it is easier to lose your place in the code. I've added a second slightly more complex example of a real world use case ofRaisedButton from the Flutter Gallery. Notice it is a lot easier to immediately spot what widget the properties are associated with. Before:

After:

Complex example -- before:

[jacob314]

MyWidget({
  CrossAxisAlignment headerAlignment = CrossAxisAlignment.center,
  Widget header,
  CrossAxisAlignment contentAlignment = CrossAxisAlignment.start,
  Widget content,
})

Interesting example @a14n. Interleaving properties with widget children is fine as long as the properties are always before the associated widget. In that case it could be reasonable to require that the named arguments are specified in the order they are specified in the MyWidget constructor which would keep the properties right next to the associated widget. Bad example showing properties after the relevant widget.

Ok examples showing properties before children.

Ok example showing properties interleaved:

[a14n]

With the UI Guides (that I don't have for now on VSCode) my complaint on the sample with onPressed almost disappear. (I didn't understand what was those stronger lines in your screenshot until I read https://medium.com/dartlang/announcing-dart-2-3-optimized-for-building-user-interfaces-e84919ca1dff)

[jacob314]

Let me know if you would like to beta-test UI Guides in VSCode when we get them working.

[Hixie]

I think this definitely helps in many cases but realistically I think there'd be too many false positives. At the end of the day, at some level, we have to let the programmer figure out what is best for them.

[jacob314]

This lint is now working well for google projects. We might want to look at creating a weakened version that is appropriate to turn on for all projects. The weakened version would allow arguments where the value is a Function to show up after child properties. As those arguments can be very large expressions, placing them first is less of a clear win particularly if the widget tree is simple.

[Yetispapa]

Is there a lint rule, to have the complete opposite, so that the child_properties are first?

[bwilkerson]

No, there isn't. Is there a motivation for such a rule?

[Yetispapa]

I learned this order back then and I apply it ever since then. The order for me is:

private properties
public properties
constructor
public functions
private functions

or

public properties
constructor
public functions
private properties
private functions

I think, back then in most schools / universities the order was always properties first and then functions and that's where i'm coming from.

Was just asking, maybe I will used to the new one :)

[bwilkerson]

I believe that there is a misunderstanding. The sort_child_properties_last lint enforces an order for named arguments in the invocation of constructors of Flutter Widget subclasses. It doesn't enforce an ordering of members in a class.

There is a 'Sort Members' command in the IDEs that will sort the members in classes, as well as top-level declarations, in a predefined order, but there is no lint to enforce that order.