Follow a consistent user model

Follow a consistent user model

[messick, 2017-05-08 21:18:28]

messick@google.com changed reviewers:
+ brianwilkerson@google.com, devoncarew@google.com

[messick, 2017-05-08 21:18:29]

The motivation for much of this work is described in the design_notes.md doc.
Basically, completion should not move the cursor backward.

[devoncarew, 2017-05-09 13:15:06]

lgtm

design notes lgtm! Nice to have those.

I am not familiar with this area of code, so you'll want to wait for Brian's
review.

https://codereview.chromium.org/2867123003/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/completion/statement/design_notes.md
(right):

https://codereview.chromium.org/2867123003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:14:
[documentatopn.](https://www.jetbrains.com/help/idea/2017.1/auto-completing-code.html#statements_completion)
sp

[messick, 2017-05-09 13:26:03]

https://codereview.chromium.org/2867123003/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/completion/statement/design_notes.md
(right):

https://codereview.chromium.org/2867123003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:14:
[documentatopn.](https://www.jetbrains.com/help/idea/2017.1/auto-completing-code.html#statements_completion)
On 2017/05/09 13:15:06, devoncarew wrote:
> sp

Done.

[Brian Wilkerson, 2017-05-09 15:12:24]

lgtm

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/services/completion/statement/design_notes.md
(right):

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:97:
for simplicity the `else` keyword is ignored. Starting with nothing
What if we find code like the following:

  if (true) {} else

Do / should we add a block for the else clause?

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:107:
the cursor will be added.
Should we add a 'break' statement at the end? I know that 'return' and 'throw'
are also allowed, but I suspect that 'break' is the most common way to end a
case clause.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:112:
for the body will be added. No clauses (n, catch, or finally) will be added.
"n" --> "on"

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
File
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart
(right):

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:314:
// multi-line string
I don't think this handles raw strings correctly.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:338:
//_addInsertEdit(loc, ',');
Remove commented out code? (Here and line 340.)

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:677:
if (node is FunctionDeclaration) {
The blocks for methods and functions appear to only depend on the parameter
list. Perhaps extract them out into a shared local function.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/tes...
File
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart
(right):

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:416:
test_withNoBody() async {
Is there a test for the following?

  class Interface {}
  class Sample implements Interface

[messick, 2017-05-09 16:32:09]

I still need to work on else blocks and raw strings so I probably won't finish
this today.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/services/completion/statement/design_notes.md
(right):

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:97:
for simplicity the `else` keyword is ignored. Starting with nothing
On 2017/05/09 15:12:23, Brian Wilkerson wrote:
> What if we find code like the following:
> 
>   if (true) {} else
> 
> Do / should we add a block for the else clause?

Probably. It does seem rude to simply ignore that case.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:107:
the cursor will be added.
On 2017/05/09 15:12:24, Brian Wilkerson wrote:
> Should we add a 'break' statement at the end? I know that 'return' and 'throw'
> are also allowed, but I suspect that 'break' is the most common way to end a
> case clause.

I don't think so. 'break' isn't required. The case could be empty, too.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/design_notes.md:112:
for the body will be added. No clauses (n, catch, or finally) will be added.
On 2017/05/09 15:12:24, Brian Wilkerson wrote:
> "n" --> "on"

Done.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
File
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart
(right):

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:314:
// multi-line string
On 2017/05/09 15:12:24, Brian Wilkerson wrote:
> I don't think this handles raw strings correctly.

You're right. Thanks for catching that.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:338:
//_addInsertEdit(loc, ',');
On 2017/05/09 15:12:24, Brian Wilkerson wrote:
> Remove commented out code? (Here and line 340.)

Done.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:677:
if (node is FunctionDeclaration) {
On 2017/05/09 15:12:24, Brian Wilkerson wrote:
> The blocks for methods and functions appear to only depend on the parameter
> list. Perhaps extract them out into a shared local function.

I made that change. I had not done it before because the local function has to
compute two values and can only return one. But, it is a local function so it
can modify the other as a side-effect, which it does.

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/tes...
File
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart
(right):

https://codereview.chromium.org/2867123003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:416:
test_withNoBody() async {
On 2017/05/09 15:12:24, Brian Wilkerson wrote:
> Is there a test for the following?
> 
>   class Interface {}
>   class Sample implements Interface

Done.

[messick, 2017-05-10 17:24:27]

PTAL

[Brian Wilkerson, 2017-05-10 17:45:39]

lgtm