OptionsValidator plugin extension and linter service.

OptionsValidator plugin extension and linter service.

* Adds a new extension point to allow for contributing OptionsValidators.
* Adds a new linter 'service' to server that contributes a lint-aware validator.

You'll notice that the warning code I added is NOT in `errors.dart`.  I did that in anticipation of plugin-specifc warnings living elsewhere.  If that's not the right angle, happy to un-do!

R=brianwilkerson@google.com

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

[pquitslund, 2015-10-23 22:03:34]

Description was changed from

==========
OptionsValidator plugin WIP.
==========

to

==========
OptionsValidator plugin extension and linter service.

* Adds a new extension point to allow for contributing OptionsValidators.
* Adds a new linter 'service' to server that contributes a lint-aware validator.

You'll notice that the warning code I added is NOT in `errors.dart`.  I did that
in anticipation of plugin-specifc warnings living elsewhere.  if that's not the
right angle, happy to un-do!
==========

[pquitslund, 2015-10-23 22:03:45]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com

[pquitslund, 2015-10-23 22:03:46]

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/options.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/options.dart:109: // TODO(pq): move to an extension
point.
If we're happy with the extension point, these can get moved to proper
contributions.

[pquitslund, 2015-10-23 22:03:59]

Description was changed from

==========
OptionsValidator plugin extension and linter service.

* Adds a new extension point to allow for contributing OptionsValidators.
* Adds a new linter 'service' to server that contributes a lint-aware validator.

You'll notice that the warning code I added is NOT in `errors.dart`.  I did that
in anticipation of plugin-specifc warnings living elsewhere.  if that's not the
right angle, happy to un-do!
==========

to

==========
OptionsValidator plugin extension and linter service.

* Adds a new extension point to allow for contributing OptionsValidators.
* Adds a new linter 'service' to server that contributes a lint-aware validator.

You'll notice that the warning code I added is NOT in `errors.dart`.  I did that
in anticipation of plugin-specifc warnings living elsewhere.  If that's not the
right angle, happy to un-do!
==========

[Brian Wilkerson, 2015-10-24 15:32:17]

LGTM

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/plugin/server_plugin.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/plugin/server_plugin.dart:328:
OPTIONS_VALIDATOR_EXTENSION_POINT_ID, new LinterRuleOptionsValidator());
Personally, I'd like to see the validator moved to the analyzer package and
registered there, but we can talk about it if you disagree.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/services/linter/linter_test.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/linter/linter_test.dart:26: new
CompositeValidator([new LinterRuleOptionsValidator()]);
Is it necessary to wrap the LinterRuleOptionsValidator in a CompositeValidator?
If not, it would be cleaner to remove it.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
File pkg/analyzer/lib/plugin/options.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
pkg/analyzer/lib/plugin/options.dart:9: import 'package:analyzer/analyzer.dart';
Whatever we're importing from here is exported from somewhere else. I'd prefer
to import it from the original library rather than indirectly.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
pkg/analyzer/lib/plugin/options.dart:76: ///
In the server plugins, I've been adding text indicating what clients can and
cannot do with the public classes. For an interface like this, the text is:

Clients may implement this class when implementing plugins.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/options.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/options.dart:109: // TODO(pq): move to an extension
point.
> If we're happy with the extension point, these can get moved to proper
> contributions.

Doesn't this CL already cause both of these to be contributed?

[pquitslund, 2015-10-26 15:48:59]

Thanks!

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/plugin/server_plugin.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/plugin/server_plugin.dart:328:
OPTIONS_VALIDATOR_EXTENSION_POINT_ID, new LinterRuleOptionsValidator());
On 2015/10/24 15:32:17, Brian Wilkerson wrote:
> Personally, I'd like to see the validator moved to the analyzer package and
> registered there, but we can talk about it if you disagree.

Let's talk about this.  My thinking is that we don't since it would add a
dependency from analyzer to linter.  I don't love it here either though.  FWIW:
this contribution won't feel lonely for too ling since there will be other
server contributions down the road -- notably fixes and completions; once we do
that maybe these registrations should move to their own plugin and we can
re-open the question of where it should best live?

Anyway, yeah, let's talk. :)

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/services/linter/linter_test.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/linter/linter_test.dart:26: new
CompositeValidator([new LinterRuleOptionsValidator()]);
On 2015/10/24 15:32:17, Brian Wilkerson wrote:
> Is it necessary to wrap the LinterRuleOptionsValidator in a
CompositeValidator?
> If not, it would be cleaner to remove it.

Nope.  And done.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
File pkg/analyzer/lib/plugin/options.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
pkg/analyzer/lib/plugin/options.dart:9: import 'package:analyzer/analyzer.dart';
On 2015/10/24 15:32:17, Brian Wilkerson wrote:
> Whatever we're importing from here is exported from somewhere else. I'd prefer
> to import it from the original library rather than indirectly.

Interesting. Maybe we should cook up a lint for this?  Or maybe it doesn't come
up often enough?

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
pkg/analyzer/lib/plugin/options.dart:76: ///
On 2015/10/24 15:32:17, Brian Wilkerson wrote:
> In the server plugins, I've been adding text indicating what clients can and
> cannot do with the public classes. For an interface like this, the text is:
> 
> Clients may implement this class when implementing plugins.

Cool.  Added.  Do you have a doc anywhere with these guidelines?  If not, maybe
added to the CONTRIBUTING doc?

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/options.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/options.dart:109: // TODO(pq): move to an extension
point.
On 2015/10/24 15:32:17, Brian Wilkerson wrote:
> > If we're happy with the extension point, these can get moved to proper
> > contributions.
> 
> Doesn't this CL already cause both of these to be contributed?

Yes.  Sorry I wasn't clear.  Notice that here the base analyzer and linter ones
are just shoved into the _validators array (unlike the one elsewhere to validate
lint rules).  When we're happy with the extension point we can discuss whether
we want the ceremony of registering them via a plugin contribution.  

I'm inclined to for consistency.  What do you think?

[pquitslund, 2015-10-26 15:50:58]

Committed patchset #3 (id:40001) manually as
68dc36a3e73c5303828c0cec9b469e2038bf7a14 (presubmit successful).

[Brian Wilkerson, 2015-10-26 16:07:14]

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/plugin/server_plugin.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/plugin/server_plugin.dart:328:
OPTIONS_VALIDATOR_EXTENSION_POINT_ID, new LinterRuleOptionsValidator());
> My thinking is that we don't since it would add a dependency from analyzer to
linter.

I agree. We don't want that dependency. But that makes me suspect that we need
to move some code from 'linter' to 'analyzer' as well. Definitely a task for
later.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
File pkg/analyzer/lib/plugin/options.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
pkg/analyzer/lib/plugin/options.dart:9: import 'package:analyzer/analyzer.dart';
> Maybe we should cook up a lint for this? Or maybe it doesn't come up often
enough?

I suspect that this doesn't come up very often. It's generally only a problem if
a type is made visible from two or more public libraries (and because of
history, that isn't the case here), which probably doesn't happen very often.
And even then, it isn't clear that it would matter which one it was imported
from. I think this is just a weird case.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/plugin...
pkg/analyzer/lib/plugin/options.dart:76: ///
> Do you have a doc anywhere with these guidelines?

No. We've been discussing the possibility of adding annotations in place of, or
in addition to, these comments.

> If not, maybe added to the CONTRIBUTING doc?

Maybe. Is it normally customized? It looks like boiler-plate to me.

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/options.dart (right):

https://codereview.chromium.org/1418333002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/options.dart:109: // TODO(pq): move to an extension
point.
> When we're happy with the extension point we can discuss whether
> we want the ceremony of registering them via a plugin contribution.  

I'm not sure we need to wait until we're happy with it (and it's part of the
public API now, so I hope we're happy with it).

> I'm inclined to for consistency.  What do you think?

I think we should use the plugin support where ever its reasonable (and I think
it's reasonable here).