Change to accommade Angular plugin and finalize removal of dependency to analysis_server

Change to accommade Angular plugin and finalize removal of dependency to analysis_server

BUG=
R=brianwilkerson@google.com, mfairhurst@google.com

Committed: https://github.com/dart-lang/sdk/commit/e57d9b608fb98a7186431e00b0f6c2fdd8c1dc3d

[maxkim, 2017-06-13 20:13:52]

Description was changed from

==========
Change to accommade Angular plugin and finalize removal of dependency to
analysis_server

BUG=
==========

to

==========
Change to accommade Angular plugin and finalize removal of dependency to
analysis_server

BUG=
==========

[maxkim, 2017-06-13 20:13:52]

maxkim@google.com changed reviewers:
+ brianwilkerson@google.com, konstantin.scheglov@gmail.com,
mfairhurst@google.com

[maxkim, 2017-06-13 20:14:23]

maxkim@google.com changed reviewers:
+ scheglov@google.com
- konstantin.scheglov@gmail.com

[mfairhurst, 2017-06-13 20:31:40]

LGTM unless we want to rearrange a bit the responsibilities: who calculates
completion targets, dot targets, and whether the contributors should have these
additional APIs, or those APIs should be put elsewhere and used by the
contributors.

Overall, this is *exactly* what we need: a way to get completion metadata
without such a wide API which may be hard to satisfy at all times (such as ours)

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart (right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart:118: if
(collector.suggestions.isEmpty) {
Should this instead check 'offsetIsSet'? And, what's the need for "no results"
to be equivalent to setting the completion offset and length to anything? Seems
reasonable, but also a bit strange.

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart:51:
Future<Null> computeSuggestionsForClass(
Could use a comment that this is here for our use case (and similar ones)

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart:59:
CompletionTarget target = new CompletionTarget.forOffset(
I would assume that we would want to pass in a CompletionTarget rather than have
it figure out its own completion target

[maxkim, 2017-06-13 20:49:22]

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart (right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart:118: if
(collector.suggestions.isEmpty) {
Ah that's right, I added this block before adding that variable. Somewhere in an
upper level, if the response has null offset/length, it crashes. Since that's
outside of the scope of where I'm comfortable contributing to, this is a
temporary yet effective bandaid.

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart:51:
Future<Null> computeSuggestionsForClass(
On 2017/06/13 20:31:39, mfairhurst wrote:
> Could use a comment that this is here for our use case (and similar ones)

Acknowledged.

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart:59:
CompletionTarget target = new CompletionTarget.forOffset(
On 2017/06/13 20:31:39, mfairhurst wrote:
> I would assume that we would want to pass in a CompletionTarget rather than
have
> it figure out its own completion target

Acknowledged.

[Brian Wilkerson, 2017-06-13 20:50:19]

I suspect that I'm missing some background that would explain why you need these
changes. The questions are intended to help me fill in the blanks.

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart:14:
class ReplacementRange {
The class ReplacementRange is defined in
analyzer_plugin/test/utilities/completion/replacement_range.dart. Do you need it
for your own tests, or are you using it in non-test code?

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart (right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart:124:
collector.offset, collector.length, collector.suggestions);
I don't understand what the length of the suggestions has to do with anything
either. Perhaps we should just pass in `collector.offset ?? request.offset,
collector.length ?? 0`?

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart:51:
Future<Null> computeSuggestionsForClass(
Is this a short-term work-around until you can just add this contributor
directly, or is this intended to be a long-term addition to the public API? In
either case, yes, it needs to be documented.

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/utilities/completion/type_member_contributor.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/type_member_contributor.dart:25:
Future<Null> computeSuggestionsWithEntryPoint(CompletionRequest request,
Ditto: Is this a short-term work-around until you can just add this contributor
directly, or is this intended to be a long-term addition to the public API? What
is the `entryPoint`?

[maxkim, 2017-06-13 21:02:00]

>LGTM unless we want to rearrange a bit the responsibilities: who calculates
>completion targets, dot targets, and whether the contributors should have these
>additional APIs, or those APIs should be put elsewhere and used by the
>contributors.

I think for at least InheritedReferenceContributor and TypeMemberContributor, we
(and potentially others) now have enough flexibility to add additional
parameters into computeSuggestions as needed by using its helper function.
(Luckily dot-target calculation can just piggy back on this without a need for
our own implementation).

Since these two are technically part of Dart Contributor - it's a finicky
use-case that we shouldn't base the design of ResolveResult or API calls for
these ad-hoc purposes. Should similar issues arise with other Dart-based
contributors, I think we should continue with ad-hoc fixes for those
contributors while developing a more universally accessible change to
ResolveResult.

[maxkim, 2017-06-13 21:11:40]

>I suspect that I'm missing some background that would explain why >you need
these
>changes. The questions are intended to help me fill in the blanks.

I've just pushed my local changes onto:
https://github.com/dart-lang/angular_analyzer_plugin/pull/371

Still need a lot of code cleanup, but I've incorporated the two
Dart-contributors into our code: 
https://github.com/mk13/angular_analyzer_plugin/blob/2bdbc1085ad0c83765baa6da...

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart:14:
class ReplacementRange {
On 2017/06/13 20:50:19, Brian Wilkerson wrote:
> The class ReplacementRange is defined in
> analyzer_plugin/test/utilities/completion/replacement_range.dart. Do you need
it
> for your own tests, or are you using it in non-test code?

We are using it in non-test cases; specifically we use it in our overloaded
version of InheritedReferenceContributor and TypeMemberContributor. Should I
adjust the dependency in the test-code while removing that class within test
folder?

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart (right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart:124:
collector.offset, collector.length, collector.suggestions);
On 2017/06/13 20:50:19, Brian Wilkerson wrote:
> I don't understand what the length of the suggestions has to do with anything
> either. Perhaps we should just pass in `collector.offset ?? request.offset,
> collector.length ?? 0`?

This is following the assumption that if suggestions is empty, then it is
probable that length and offset hasn't been set. I will change it to ??=
assignments.

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/inherited_reference_contributor.dart:51:
Future<Null> computeSuggestionsForClass(
This is intended to be long-term and I'll write up an API documentation.

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/utilities/completion/type_member_contributor.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/utilities/completion/type_member_contributor.dart:25:
Future<Null> computeSuggestionsWithEntryPoint(CompletionRequest request,
On 2017/06/13 20:50:19, Brian Wilkerson wrote:
> Ditto: Is this a short-term work-around until you can just add this
contributor
> directly, or is this intended to be a long-term addition to the public API?
What
> is the `entryPoint`?

EntryPoint is used as an optional parameter in computeTarget that we need to
utilize since we directly provide the Dart-based AstNode from parsing Angular
HTML - specifically in expressions.

[Brian Wilkerson, 2017-06-13 22:49:25]

lgtm

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
File pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart
(right):

https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart:14:
class ReplacementRange {
> Should I adjust the dependency in the test-code while removing that
> class within test folder?

Effectively, yes. (I'd actually prefer to keep the content of the file in the
test directory because I fixed a few typos in comments, but it's not that big an
issue.)

However, any code you use from analyzer_plugin should be outside of `src`, so
this file would need to move (eventually) too.

[maxkim, 2017-06-13 23:01:42]

On 2017/06/13 22:49:25, Brian Wilkerson wrote:
> lgtm
> 
>
https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
> File pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart
> (right):
> 
>
https://codereview.chromium.org/2934133002/diff/60001/pkg/analyzer_plugin/lib...
> pkg/analyzer_plugin/lib/src/utilities/completion/replacement_range.dart:14:
> class ReplacementRange {
> > Should I adjust the dependency in the test-code while removing that
> > class within test folder?
> 
> Effectively, yes. (I'd actually prefer to keep the content of the file in the
> test directory because I fixed a few typos in comments, but it's not that big
an
> issue.)
> 
> However, any code you use from analyzer_plugin should be outside of `src`, so
> this file would need to move (eventually) too.

Sure sounds good, I'll adjust these files in a separate CL (since you'll be away
by the time I finish it) and you can review that CL at your convenience. I would
prefer to get this CL in today and I feel that this change is big enough to need
another lgtm.

[maxkim, 2017-06-13 23:31:26]

Description was changed from

==========
Change to accommade Angular plugin and finalize removal of dependency to
analysis_server

BUG=
==========

to

==========
Change to accommade Angular plugin and finalize removal of dependency to
analysis_server

BUG=
R=brianwilkerson@google.com, mfairhurst@google.com

Committed:
https://github.com/dart-lang/sdk/commit/e57d9b608fb98a7186431e00b0f6c2fdd8c1dc3d
==========

[maxkim, 2017-06-13 23:31:27]

Committed patchset #5 (id:80001) manually as
e57d9b608fb98a7186431e00b0f6c2fdd8c1dc3d (presubmit successful).