Completions on named arguments should include trailing comma

[mit-mit]

Repro steps:

1. Create a new Flutter project in IJ
2. In main.dart place the cursor at the end of line 77 ( title: new Text(config.title),)
3. Press Enter to add a new line
4. Type back and then Enter to select the backgroundColor completion

Expected result:

• Editor completes to backgroundColor: , and the caret is placed just before ,
• The completed line is properly indented

Actual result:

• Editor adds just backgroundColor: resulting in the completed code triggering an analyzer error
• Line is not properly indented

[mit-mit]

I speculate this was not done before as the last named argument was not allowed to have a terminating comma, but now that this is allowed I think this would be very valuable.

[devoncarew]

A thought (from @mit-mit): what if we always insert trailing commas for methods w/ named params w/ two or more args?

This would be for when you're calling a ctor and you are setting 2 or more params. Likely also for lists.

[pq]

WIP: JetBrains/intellij-plugins#520

[alexander-doroshko]

Actual result: Line is not properly indented

This seems to be already fixed, can you please double check?

Actual result: Editor adds just backgroundColor: resulting in the completed code triggering an analyzer error

With auto-inserted comma or without it analyzer is giving an error identifier expected which absolutely expected in this case and it disappears as soon as you type in the arg value, regardless of the comma presence.

Expected result: Editor completes to backgroundColor: , and the caret is placed just before ,

Phil's PR is almost ready, but before finishing it up and landing I'd like to understand the rationale behind this feature. Sorry, I failed to guess what becomes better with this feature :).

[pq]

I'll take a quick stab but hopefully @mit-mit will chime in.

A compelling benefit in ensuring trailing commas for Flutter is in how they trigger the formatter. The formatter is trained to handle trailing commas in such a way that makes formatted constructor invocations that include lots of nested args look like idiomatic Flutter if there are trailing commas. For example, compare with trailing commas,

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return new MaterialApp(
      title: 'Flutter Demo',
      theme: new ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: new MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

to without:

  @override
  Widget build(BuildContext context) {
    return new MaterialApp(
        title: 'Flutter Demo',
        theme: new ThemeData(
          primarySwatch: Colors.blue,
        ),
        home: new MyHomePage(title: 'Flutter Demo Home Page'));
  }

The nested structure is clearer in the first.

[alexander-doroshko]

Thanks, makes sense.

I have good news! You do not need any PR to land in the Dart plugin to get this functionality.
Try steps from this issue description, but one line before title: new Text(config.title),. You'll see that for this case completion suggestions from the Analysis Server already include trailing commas:

I think it would be suboptimal if some trailing commas are provided by the analysis server, but others - by the IDE. I guess you'd better improve flutter-specific completion at server side.

You'll notice though that the caret is placed after comma on such item selection, whereas the caret before comma is expected. This also seems to be a server-side issue. It sends incorrect CompletionSuggestion.selectionOffset. CompletionSuggestion.selectionLength is normally equal to 0 and this selectionOffset is used to place the caret on item insertion.

[pq]

OK, great. We agree!

I also think this would better implemented on the server side. 👍 I talked myself out of it since I thought we might want a preference and that lead to me thinking it would all be easier on the client side. But anyway, I agree, so I'll go back to that approach since it has a bunch of other advantages.

As for your experience, I'm curious what version of the SDK you have. I'm on 1.23.0-dev.11.7 and not seeing the trailing commas:

[alexander-doroshko]

tried the freshest sdk and brand new project

Flutter • channel master • https://github.com/flutter/flutter.git
Framework • revision 21f57a85e8 (14 hours ago) • 2017-04-25 17:40:28 -0700
Engine • revision a5b64899c9 
Tools • Dart 1.23.0-dev.11.11

[alexander-doroshko]

however, even the old Dart SDK 1.19 does the same

[mit-mit]

The behavior appears to differ depending on whether you complete on "nothing" (ctrl-space) or on an actual completion such as 'b' in Phil's screenshot?

[alexander-doroshko]

I thought we might want a preference ... easier on the client side

We may want a preference and may implement all on the client side. But we should agree with server first. What I think we definitely wouldn't like to have is to have trailing commas in some cases coming from the server, in other cases - added by client.

The behavior appears to differ depending on whether you complete on "nothing" (ctrl-space) or on an actual completion such as 'b'

Wow, indeed! Not sure what rationale is behind.

[pq]

What I think we definitely wouldn't like to have is to have trailing commas in some cases coming from the server, in other cases - added by client.

👍 I'll look into this as I move towards implementing this support on the server side.

[pq]

WIP (https://codereview.chromium.org/2839273002/):

[pq]

Implemented w/ dart-lang/sdk@bfc071e.

[mit-mit]

@pq when will we see this in the product?

[pq]

The next push of the flutter SDK.