[lint] cascade_invocations

[evanweible-wf]

http://dart-lang.github.io/linter/lints/cascade_invocations.html

[greglittlefield-wf]

I've found that this can negatively impact readability in certain cases, and be annoying to deal with in other cases, and question whether making this recommended provides significant benefit.

I propose removing this as a recommended lint.

Some examples from wdesk_sdk, with the diff showing what it would take to satisfy the lint.

1. Performing an action on the result of the call. If you're forced to cascade it, it shows up on the same line, which in some cases IMO isn't as readable.

      _handleRemovePanelItem(PanelModelContext context) {
   -    var panel = _getPanel(context);
   -    panel._removePanelItem(context);
   +    var panel = _getPanel(context).._removePanelItem(context);
   
        if (panel.models.isEmpty) {
          _resizePanels(shouldTransition: false);
        }
      }

2. When performing a series of calls, interleaved with other calls, you end up sometimes having to cascade things and sometimes not, making moving around these calls during development a pain. Also, for cascades of two items, you end up with code that's a line longer.

    action = AddEditorAction('cssNamespace', parentId: rootEditor);
    childEditorA = action.editorId;
    store.dispatch(action);
   
    action = AddEditorAction('cssNamespace', parentId: rootEditor);
    childEditorB = action.editorId;
   -    store.dispatch(action);
   -    store.dispatch(SetActiveChildEditorAction(rootEditor, childEditorA));
   +    store
   +      ..dispatch(action);
   +      ..dispatch(SetActiveChildEditorAction(rootEditor, childEditorA));

[dustyholmes-wf]

I recognize that this lint may be arbitrary and a little heavy for recommended. I opted to enforce it in DPC because of these reasons.

1. When you change a variable name, it has fewer instances that must be updated.

-    stoore     
+    store
      ..dispatch(action);
      ..dispatch(SetActiveChildEditorAction(rootEditor, childEditorA));

2. It is a language feature that helps us completely avoid having instances floating around longer than necessary. When implementing this lint in the past I discovered many instances of this that people just couldn't see without the lint.

-   final store = Store();
-   store.dispatch(action);
+   Store()..dispatch(action);

Despite the fact that it truly is a syntax preference, I would prefer keeping it as a recommended lint.

[greglittlefield-wf]

Thanks for the discussion, @dustyholmes-wf!

1. When you change a variable name, it has fewer instances that must be updated.

That's a fair point, but also Dart IDEs can automate variable renaming, so developers don't have to manually update each instance. They're also free to take that opportunity to refactor the calls into a cascade.

2. It is a language feature that helps us completely avoid having instances floating around longer than necessary. When implementing this lint in the past I discovered many instances of this that people just couldn't see without the lint.

I would expect it to be relatively rare to have cases of instantiating an object only to call an instance method on it and discard both the instance and the result of the method call. Do you have examples of this?

It doesn't seem clear-cut that the benefits of this lint outweigh the drawbacks, so to me it doesn't seem appropriate to put in recommended. That doesn't stop specific repos from adding it to their sets of lints, though.

[dustyholmes-wf]

@greglittlefield-wf my concern for 1 was more on the review side than on the edit side. It's a small thing, but helping people make smaller changes has been top of my mind.

As for examples:

When we need to make multiple calls into APIs.

void _updateToolbar([_]) {
-    final edit = _toolbarModule.api.edit;

-    edit.setStyle(_formatTags.getString(TableFormatTypes.styleKey));
-    edit.setFontFamily(_formatTags.getString(TableFormatTypes.fontFamily, 'Arial'));
-    edit.setIsBold(_formatTags.getBool(TableFormatTypes.bold));
-    edit.setIsItalic(_formatTags.getBool(TableFormatTypes.italic));
-    edit.setIsUnderline(_formatTags.getBool(TableFormatTypes.underline));
-    edit.setIsStrikethrough(_formatTags.getBool(TableFormatTypes.strike));
-    edit.setDisplayValue(_formatTags.displayValue);
-    edit.setSelectionTextColor(_formatTags.getString(TableFormatTypes.textColor));
-    edit.setSelectionBackgroundColor(_formatTags.getString(TableFormatTypes.backgroundColor));
-    edit.setCharacterSpacing(_formatTags.getInt(TableFormatTypes.characterSpacing, 0));

+    _toolbarModule.api.edit
+       ..setStyle(_formatTags.getString(TableFormatTypes.styleKey))
+       ..setFontFamily(_formatTags.getString(TableFormatTypes.fontFamily, 'Arial'))
+       ..setIsBold(_formatTags.getBool(TableFormatTypes.bold))
+       ..edit.setIsItalic(_formatTags.getBool(TableFormatTypes.italic))
+       ..edit.setIsUnderline(_formatTags.getBool(TableFormatTypes.underline))
+       ..edit.setIsStrikethrough(_formatTags.getBool(TableFormatTypes.strike))
+       ..edit.setDisplayValue(_formatTags.displayValue)
+       ..edit.setSelectionTextColor(_formatTags.getString(TableFormatTypes.textColor))
+       ..edit.setSelectionBackgroundColor(_formatTags.getString(TableFormatTypes.backgroundColor))
+       ..edit.setCharacterSpacing(_formatTags.getInt(TableFormatTypes.characterSpacing, 0))
  }

When we're constructing a child object or parameter.
```diff
- final so = util.waterfallWithTotalColumnSeriesOptions(mgr, 's5', 2, 4, null);
- so.dataLabels = highcharts.DataLabels()..enabled = true;
- final dil = util.di(so);

+ final dil = util.di(
+   util.waterfallWithTotalColumnSeriesOptions(mgr, 's5', 2, 4, null)
+     ..dataLabels = (highcharts.DataLabels()..enabled = true),
+ )

I turned this on in a few packages and found that about 1/3 of the instances could avoid a variable entirely.

[evanweible-wf]

We talked more about this one internally and it was pretty 50/50. We're going to remove it from the v2 recommended ruleset in #182 and teams can choose to independently enable it if desired.