add analysis server --flutter-repo startup flag

add analysis server --flutter-repo startup flag

This adds a new flag used by `flutter analyze --watch` when analyzing
the flutter repository. When specified, this flag causes analysis server
to enable the public_member_api_docs lint even though it is not enabled
in the analysis_options file. `flutter analyze --watch` needs the results
of this lint to summarize the number of public members missing dartdoc,
but enabling it in the analysis_options causes too much noise in the IDE.

See https://github.com/flutter/flutter/issues/10721 for more background

R=devoncarew@google.com

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

[danrubel, 2017-07-08 10:37:01]

danrubel@google.com changed reviewers:
+ devoncarew@google.com, pquitslund@google.com

[devoncarew, 2017-07-10 16:47:35]

lgtm

Some suggestions inline -

https://codereview.chromium.org/2976473003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/server/driver.dart (right):

https://codereview.chromium.org/2976473003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/server/driver.dart:320:
ContextBuilderOptions.flutterRepo = results[FLUTTER_REPO];
No real opinion here, but many of the analysis server options are now being
passed around in the `analysisServerOptions` object. If it's convenient here, it
might be nice to centralize around that.

https://codereview.chromium.org/2976473003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/server/driver.dart:497: parser.addFlag(FLUTTER_REPO,
hide: true);
This is the only option we're hiding; perhaps don't hide it? It's not a
user-facing flag, but the cli for the analysis server isn't generally run by
hand.

https://codereview.chromium.org/2976473003/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/builder.dart (right):

https://codereview.chromium.org/2976473003/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/builder.dart:464:
options.lintRules.firstWhere((Linter lint) => lint.name == lintName,
Perhaps restructure this a bit so that you're not doing mutation in the orElse?
It could be unexpected when people read this code.

https://codereview.chromium.org/2976473003/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/builder.dart:625: static bool flutterRepo = false;
This could go away if we move to storing the flag on the AnalysisOptions object.

[danrubel, 2017-07-10 20:47:59]

https://codereview.chromium.org/2976473003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/server/driver.dart (right):

https://codereview.chromium.org/2976473003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/server/driver.dart:320:
ContextBuilderOptions.flutterRepo = results[FLUTTER_REPO];
On 2017/07/10 16:47:35, devoncarew wrote:
> No real opinion here, but many of the analysis server options are now being
> passed around in the `analysisServerOptions` object. If it's convenient here,
it
> might be nice to centralize around that.

I definitely considered that because I too like that better, but the
AnalysisServerOptions are not available where I need them in analyzer's
ContextBuilderOptions. I'm open to suggestions and happy to refactor after the
fact.

https://codereview.chromium.org/2976473003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/server/driver.dart:497: parser.addFlag(FLUTTER_REPO,
hide: true);
On 2017/07/10 16:47:35, devoncarew wrote:
> This is the only option we're hiding; perhaps don't hide it? It's not a
> user-facing flag, but the cli for the analysis server isn't generally run by
> hand.

Done.

https://codereview.chromium.org/2976473003/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/builder.dart (right):

https://codereview.chromium.org/2976473003/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/builder.dart:464:
options.lintRules.firstWhere((Linter lint) => lint.name == lintName,
On 2017/07/10 16:47:35, devoncarew wrote:
> Perhaps restructure this a bit so that you're not doing mutation in the
orElse?
> It could be unexpected when people read this code.

Done.

https://codereview.chromium.org/2976473003/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/builder.dart:625: static bool flutterRepo = false;
On 2017/07/10 16:47:35, devoncarew wrote:
> This could go away if we move to storing the flag on the AnalysisOptions
object.

I didn't want to add this flag to AnalysisOptions because I didn't want this
flag to become public and easily used. It's an internal implementation detail.
That said I don't feel strongly about it and happy to refactor after the fact.

[danrubel, 2017-07-10 20:50:13]

Description was changed from

==========
add analysis server --flutter-repo startup flag

This adds a new flag used by `flutter analyze --watch` when analyzing
the flutter repository. When specified, this flag causes analysis server
to enable the public_member_api_docs lint even though it is not enabled
in the analysis_options file. `flutter analyze --watch` needs the results
of this lint to summarize the number of public members missing dartdoc,
but enabling it in the analysis_options causes too much noise in the IDE.

See https://github.com/flutter/flutter/issues/10721 for more background
==========

to

==========
add analysis server --flutter-repo startup flag

This adds a new flag used by `flutter analyze --watch` when analyzing
the flutter repository. When specified, this flag causes analysis server
to enable the public_member_api_docs lint even though it is not enabled
in the analysis_options file. `flutter analyze --watch` needs the results
of this lint to summarize the number of public members missing dartdoc,
but enabling it in the analysis_options causes too much noise in the IDE.

See https://github.com/flutter/flutter/issues/10721 for more background

R=devoncarew@google.com

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

[danrubel, 2017-07-10 20:50:15]

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

[Brian Wilkerson, 2017-07-19 16:44:36]

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

[Brian Wilkerson, 2017-07-19 16:44:37]

I have a concern with this approach. Specifically, it increases the complexity
of the algorithm used to determine which lint rules are enabled by adding yet
another avenue for enabling some rules. The algorithm was already complex enough
to cause some amount of user confusion, so increasing the complexity seems like
the wrong approach. (Yes, I know that most users won't see the complexity for
this specific use case, but I'm concerned that it establishes a precedent that
will be abused in the future.)

I would love to know which other options, if any, were explored and why they
were rejected. For example, could `flutter analyze` create a temporary analysis
options file that includes the base options file?

[danrubel, 2017-07-19 17:41:37]

On 2017/07/19 16:44:37, Brian Wilkerson wrote:
> I have a concern with this approach. Specifically, it increases the complexity
> of the algorithm used to determine which lint rules are enabled by adding yet
> another avenue for enabling some rules. The algorithm was already complex
enough
> to cause some amount of user confusion, so increasing the complexity seems
like
> the wrong approach. (Yes, I know that most users won't see the complexity for
> this specific use case, but I'm concerned that it establishes a precedent that
> will be abused in the future.)
> 
> I would love to know which other options, if any, were explored and why they
> were rejected. For example, could `flutter analyze` create a temporary
analysis
> options file that includes the base options file?

I agree with your concerns and would like a better approach if one exists.
Let's chat offline to see if we can come up with something better.