Add the analysis options path to the diagnostics page.

Add the analysis options path to the diagnostics page.

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

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

[devoncarew, 2017-05-27 18:04:08]

devoncarew@google.com changed reviewers:
+ brianwilkerson@google.com, danrubel@google.com, paulberry@google.com

[devoncarew, 2017-05-27 18:04:09]

An alternative to https://codereview.chromium.org/2912573002/, this instead
captures the file path we read the analysis options from, and displays that in
the analysis server's diagnostics page.

[Paul Berry, 2017-05-28 04:19:15]

IMHO it's a little weird to use AnalysisOptions to communicate data back up to
the user--it's an unexpected direction of data flow and I think that might make
it confusing for future code maintainers.

But assuming Brian is ok with it, lgtm.

https://codereview.chromium.org/2913493002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler2.dart (right):

https://codereview.chromium.org/2913493002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler2.dart:519: }
Add an else block that writes '<p>none</p>', lke we do in the lints.isEmpty case
above (see line 495)

[Brian Wilkerson, 2017-05-28 14:38:22]

I agree with Paul.

The good news is that there's a good place to add the information: ContextRoot.
An instance is passed to ContextBuilder.buildDriver (so the path can be written)
and is available via AnalysisDriver.contextRoot (so that it can be read). As a
bonus, if we put the information there, then we could pass it to plugins so that
they don't need to re-perform the look-up algorithm and we can keep them more
consistent.

[devoncarew, 2017-05-30 04:01:21]

On 2017/05/28 14:38:22, Brian Wilkerson wrote:
> I agree with Paul.
> 
> The good news is that there's a good place to add the information:
ContextRoot.
> An instance is passed to ContextBuilder.buildDriver (so the path can be
written)
> and is available via AnalysisDriver.contextRoot (so that it can be read). As a
> bonus, if we put the information there, then we could pass it to plugins so
that
> they don't need to re-perform the look-up algorithm and we can keep them more
> consistent.

Hmm, I think of the path the options were read from as an intrinsic (but
optional) property of the analysis options themselves. It's not data that
effects analysis, but is useful diagnostic data to show users. Putting it on the
ContextRoot would be de-normalizing the analysis options object somewhat.

In any case, re-swizzled to put the optionsFilePath field on the ContextRoot,
and added a test to verify that calling ContextBuilder.getAnalysisOptions()
populates that field.

[Brian Wilkerson, 2017-05-30 14:26:39]

lgtm

> ... I think of the path the options were read from as an
> intrinsic (but optional) property of the analysis
> options themselves. It's not data that effects analysis ...

Given that it doesn't effect analysis, I *don't* think of it as an analysis
option. Hence it makes sense to me to put it on ContextRoot. Just a different
perspective, I guess.

https://codereview.chromium.org/2913493002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/status/get_handler2.dart (right):

https://codereview.chromium.org/2913493002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/status/get_handler2.dart:518:
'<p>${HTML_ESCAPE.convert(driver.contextRoot.optionsFilePath)}</p>');
An 'else' that writes 'none' would be good here.

https://codereview.chromium.org/2913493002/diff/20001/pkg/analyzer/lib/contex...
File pkg/analyzer/lib/context/context_root.dart (right):

https://codereview.chromium.org/2913493002/diff/20001/pkg/analyzer/lib/contex...
pkg/analyzer/lib/context/context_root.dart:28: * from. This value can be `null`.
Add a reason for 'null', such as "if there is no analysis options file or if the
location of the file has not yet been discovered".

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

https://codereview.chromium.org/2913493002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/builder.dart:394: {void verbosePrint(String text),
ContextRoot contextRoot}) {
Are there invocations where we won't have a ContextRoot to pass in, or is this
optional to prevent making a breaking change?

[devoncarew, 2017-05-30 20:23:32]

updated - ptal

https://codereview.chromium.org/2913493002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler2.dart (right):

https://codereview.chromium.org/2913493002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler2.dart:519: }
On 2017/05/28 04:19:15, Paul Berry wrote:
> Add an else block that writes '<p>none</p>', lke we do in the lints.isEmpty
case
> above (see line 495)

Done.

https://codereview.chromium.org/2913493002/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/status/get_handler2.dart (right):

https://codereview.chromium.org/2913493002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/status/get_handler2.dart:518:
'<p>${HTML_ESCAPE.convert(driver.contextRoot.optionsFilePath)}</p>');
On 2017/05/30 14:26:39, Brian Wilkerson wrote:
> An 'else' that writes 'none' would be good here.

Done.

https://codereview.chromium.org/2913493002/diff/20001/pkg/analyzer/lib/contex...
File pkg/analyzer/lib/context/context_root.dart (right):

https://codereview.chromium.org/2913493002/diff/20001/pkg/analyzer/lib/contex...
pkg/analyzer/lib/context/context_root.dart:28: * from. This value can be `null`.
On 2017/05/30 14:26:39, Brian Wilkerson wrote:
> Add a reason for 'null', such as "if there is no analysis options file or if
the
> location of the file has not yet been discovered".

Done.

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

https://codereview.chromium.org/2913493002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/builder.dart:394: {void verbosePrint(String text),
ContextRoot contextRoot}) {
On 2017/05/30 14:26:39, Brian Wilkerson wrote:
> Are there invocations where we won't have a ContextRoot to pass in, or is this
> optional to prevent making a breaking change?

There are places where we don't have a ContextRoot (or, it's not clear to me
where I would find it). This is for code that calls getAnalysisOptions()
directly. For code sites that call buildDriver(), we always have a contextRoot.

[Brian Wilkerson, 2017-05-30 20:27:03]

lgtm

[devoncarew, 2017-05-30 20:29:28]

Description was changed from

==========
Add the analysis options path to the diagnostics page.

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

to

==========
Add the analysis options path to the diagnostics page.

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

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

[devoncarew, 2017-05-30 20:29:29]

Committed patchset #4 (id:60001) manually as
1e7ff6bccfca66159165218c944ee4628facf17b (presubmit successful).