Update the plugin API

Update the plugin API

R=maxkim@google.com, mfairhurst@google.com

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

[Brian Wilkerson, 2017-06-22 17:12:20]

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

[Brian Wilkerson, 2017-06-22 17:12:21]

This turned out to be a bigger change than I had guessed. Let me know if there's
even more work needed to accommodate the angular plugin's needs.

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart:96: GeneratorResult
generateFixesResponse(DartFixesRequest request) {
I'm not entirely thrilled with this change. The alternative is to extend
FixesRequest to have getters for 'errors' and 'lineInfo'. Thoughts appreciated.

[maxkim, 2017-06-22 20:52:57]

Thanks a lot for revamping the system - I know it's a lot to ask but this new
structure is providing a lot of customization while holding some of the critical
elements of the Plugin. 

Seems like the main issue is with FixesRequest object - in which I agree with
you that the very baseline FixesRequest object SHOULD hold an errors list.

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/assist/assist.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/assist/assist.dart:28: class
AssistRequestImpl implements DartAssistRequest {
DartAssistRequestImpl

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/completion/completion_core.dart
(right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/completion/completion_core.dart:79: class
CompletionRequestImpl implements DartCompletionRequest {
DartCompletionRequestImpl

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/fixes/fixes.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/fixes/fixes.dart:44: class
FixesRequestImpl implements DartFixesRequest {
Rename this to DartFixesRequestImpl?

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/navigation/navigation.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/navigation/navigation.dart:90: class
NavigationRequestImpl implements DartNavigationRequest {
DartNavigationRequestImpl

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/assist/assist.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/assist/assist.dart:82: abstract class
AssistRequest {
Add String path.

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/completion/completion_core.dart:132: abstract
class CompletionRequest {
Add String path.

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart:57: abstract class
FixesRequest {
Can we also add 'String path' to this object?

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart:96: GeneratorResult
generateFixesResponse(DartFixesRequest request) {
On 2017/06/22 17:12:21, Brian Wilkerson wrote:
> I'm not entirely thrilled with this change. The alternative is to extend
> FixesRequest to have getters for 'errors' and 'lineInfo'. Thoughts
appreciated.

Adding a 'List<AnalysisError> get errorsAtOffset' makes sense in this context
and let the plugin-developer define how the list of errors is obtained. However,
I'm more in favor of just storing the errors for the offset instead of all
errors in the file.

Naming wise, it would be nice to have additional clarity in the field names: 

AnalysisError get fixedError; 
String path;
int get offset;
ResourceProvider get resourceProvider;
List<AnalysisError> get errorsAtOffset;  // Returns all error in file at offset

Meanwhile LineInfo can just exist in DartFixesRequest.

This way we can set 'generateFixesResponse' to take in a 'FixesRequest request'
instead of DartFixesRequest. Ideally no one should have to overload this
function.

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart:119: Iterable<AnalysisError>
_getErrors(DartFixesRequest request) {
If we put 'errorsAtOffset' within FixesRequest, this entire logic can
effectively be moved into
DartFixesMixin -> getFixesRequest(...).

And (in generateFixesResponse(...)) errors = _getErrors(request) can just become
errors = request.errors

This way we can even remove LineInfo from FixesRequest dependency from the root
FixesRequest object.

[Brian Wilkerson, 2017-06-22 22:07:01]

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/assist/assist.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/assist/assist.dart:28: class
AssistRequestImpl implements DartAssistRequest {
Done

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/completion/completion_core.dart
(right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/completion/completion_core.dart:79: class
CompletionRequestImpl implements DartCompletionRequest {
Done

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/fixes/fixes.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/fixes/fixes.dart:44: class
FixesRequestImpl implements DartFixesRequest {
Done

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
File pkg/analyzer_plugin/lib/src/utilities/navigation/navigation.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/src...
pkg/analyzer_plugin/lib/src/utilities/navigation/navigation.dart:90: class
NavigationRequestImpl implements DartNavigationRequest {
Done

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/assist/assist.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/assist/assist.dart:82: abstract class
AssistRequest {
Is that something angular needs?

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart:96: GeneratorResult
generateFixesResponse(DartFixesRequest request) {
Yeah, I'm starting to think that you're right, and that `FixesRequest` should
have an `errorsToFix` getter that returns exactly the errors for which fixes
should be produced and that fix contributors should iterate over the list. (The
current implementation in analysis server moves the iteration outside the
contributors, and I was trying to keep that, but I don't think it's worth the
pain it's causing.)

[maxkim, 2017-06-22 23:18:20]

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/assist/assist.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/assist/assist.dart:82: abstract class
AssistRequest {
On 2017/06/22 22:07:01, Brian Wilkerson wrote:
> Is that something angular needs?

Hmmm I guess it doesn't have to belong in the base class. Sorry - I was
overthinking it. If we needed it we can just load it in our version of Request
object.

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
File pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/uti...
pkg/analyzer_plugin/lib/utilities/fixes/fixes.dart:96: GeneratorResult
generateFixesResponse(DartFixesRequest request) {
On 2017/06/22 22:07:01, Brian Wilkerson wrote:
> Yeah, I'm starting to think that you're right, and that `FixesRequest` should
> have an `errorsToFix` getter that returns exactly the errors for which fixes
> should be produced and that fix contributors should iterate over the list.
(The
> current implementation in analysis server moves the iteration outside the
> contributors, and I was trying to keep that, but I don't think it's worth the
> pain it's causing.)

Acknowledged.

[mfairhurst, 2017-06-22 23:47:31]

lgtm

LGTM! Looks like you guys came up with a fairly small, very flexible set of API
changes. Wholly approve from what I can tell so far!

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/plu...
File pkg/analyzer_plugin/lib/plugin/navigation_mixin.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/plu...
pkg/analyzer_plugin/lib/plugin/navigation_mixin.dart:26: abstract class
DartNavigationMixin implements NavigationMixin {
Should it extend NavigationMixin so many people can just do

class Plugin extends ServerPlugin with DartNavigationMixin {...} rather than
with NavigationMixin, DartNavigationMixin {...}

[Brian Wilkerson, 2017-06-23 15:02:17]

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/plu...
File pkg/analyzer_plugin/lib/plugin/navigation_mixin.dart (right):

https://codereview.chromium.org/2953093002/diff/1/pkg/analyzer_plugin/lib/plu...
pkg/analyzer_plugin/lib/plugin/navigation_mixin.dart:26: abstract class
DartNavigationMixin implements NavigationMixin {
Sigh. I already tried that, but it can't. A class cannot be used as a mixin if
it extends anything other than Object.

[Brian Wilkerson, 2017-06-23 15:31:24]

I have updated FixesRequest. PTAL

[maxkim, 2017-06-23 19:08:54]

lgtm

[Brian Wilkerson, 2017-06-23 19:31:05]

Description was changed from

==========
Update the plugin API
==========

to

==========
Update the plugin API

R=maxkim@google.com, mfairhurst@google.com

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

[Brian Wilkerson, 2017-06-23 19:31:07]

Committed patchset #2 (id:20001) manually as
4a10003924165dd97497d3bc7525b921f37ab4d3 (presubmit successful).

[scheglov, 2017-06-26 15:36:45]

LGTM