Unsupported analysis option validation.

Unsupported analysis option validation.

R=scheglov@google.com

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

[pquitslund, 2015-10-21 20:54:12]

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

[pquitslund, 2015-10-21 20:54:13]

Note the TODO to surface an extension point to contribute validators.  Unless
you disagree that we should defer to plugins to do their own validation, we
should discuss an API. Needless to say, `OptionsValidator` as it stands is a
little inelegant.

[scheglov, 2015-10-21 21:01:24]

LGTM

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error.dart:350: ErrorType get type =>
ErrorType.COMPILE_TIME_ERROR;
STATIC_WARNING?

[pquitslund, 2015-10-21 21:04:51]

Thanks!

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error.dart:350: ErrorType get type =>
ErrorType.COMPILE_TIME_ERROR;
On 2015/10/21 21:01:24, scheglov wrote:
> STATIC_WARNING?

Done.

[Brian Wilkerson, 2015-10-21 21:13:58]

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error.dart (left):

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error.dart:313: * Initialize a newly created
error code to have the given [name]. The message
Not sure why you'd want to drop the documentation. As far as I can see it's
still accurate.

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
File pkg/analyzer/lib/src/task/options.dart (right):

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
pkg/analyzer/lib/src/task/options.dart:102: AnalysisError _unsupportedOption(
Consider making this utility method a public instance method on OptionsValidator
so that validators that are not implemented in this library can also use it.

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
pkg/analyzer/lib/src/task/options.dart:111: List<AnalysisError> validate(Source
source, Map<String, YamlNode> options);
Consider passing in an ErrorReporter rather than the Source.

https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
pkg/analyzer/lib/src/task/options.dart:124: YamlNode node = options['analyzer'];
Make 'analyzer' a constant?

[pquitslund, 2015-10-21 21:14:04]

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

[pquitslund, 2015-10-21 21:29:28]

Thanks for the comments.  I'll address in a follow-up CL.

On 2015/10/21 21:13:58, Brian Wilkerson wrote:
>
https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/error.dart (left):
> 
>
https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/error.dart:313: * Initialize a newly created
> error code to have the given [name]. The message
> Not sure why you'd want to drop the documentation. As far as I can see it's
> still accurate.

Sorry!  Looked like boilerplate to me...  Shall I add back?

> 
>
https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
> File pkg/analyzer/lib/src/task/options.dart (right):
> 
>
https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
> pkg/analyzer/lib/src/task/options.dart:102: AnalysisError _unsupportedOption(
> Consider making this utility method a public instance method on
OptionsValidator
> so that validators that are not implemented in this library can also use it.

Agreed.  Meta-point: we should talk about a nice API here.  For example, I don't
think validators need to know about Sources but we need them to create errors.

> 
>
https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
> pkg/analyzer/lib/src/task/options.dart:111: List<AnalysisError>
validate(Source
> source, Map<String, YamlNode> options);
> Consider passing in an ErrorReporter rather than the Source.
> 
>
https://codereview.chromium.org/1418533004/diff/1/pkg/analyzer/lib/src/task/o...
> pkg/analyzer/lib/src/task/options.dart:124: YamlNode node =
options['analyzer'];
> Make 'analyzer' a constant?

Absolutely.  I'm thinking we could have a base class that handles this general
case (plugins with a list of top-level options).  Again: this is part of the API
conversation.