Plumb page width from analysis_options.yaml to formatter when formatting in analysis_server

[munificent]

My understanding is that analysis_server uses the formatter directly as a library so that it can format code as it exists in the IDE and not on disk.

I recently added support to the dart format CLI (but not the dart_style library API) to look for a surrounding analysis_options.yaml file and configure the page width from there if present. The schema looks like:

formatter:
  page_width: 123

To be consistent, analysis_server should also plumb that same page width in when it formats a file in memory. The library API already supports a pageWidth optional parameter, so this can be implemented whenever.

Let me know if you have any questions and I can fill you in on all the details. If you want a reference, the dart format implementation is here with these tests. (I harvested this code from analyzer initially, but ended up rewriting a lot because the YamlNode API is... weird.)

Thank you!

cc @DanTup

[DanTup]

@bwilkerson @munificent both the LSP and legacy server have ways to configure page width already - which should take priority when both are provided?

My feeling is that analysis_options.yaml should be priority, so if in VS Code you set your own default to 120 but clone a project using 100, you will get 100. However, in the legacy protocol the value is passed to the individual edit.format call and so ignoring that value (because there's one in analysis_options) seems slightly weirder.

I presume the rules for overriding analysis_options.yaml in dart format are the same as what's been implemented for the server - eg. an analysis_options.yaml completely prevents use of values from any ancestor unless it was included? I think these tests confirm that, although I couldn't see a test that uses include and specifically checks the pageWidth.

Also (since we'll need to validate the options):

1. is this valid?

formatter:
  page_width:

2. Is 0 valid?

TODO

• validate options (https://dart-review.googlesource.com/c/sdk/+/388822)
• code completion for options (https://dart-review.googlesource.com/c/sdk/+/388822)
• implement in LSP (https://dart-review.googlesource.com/c/sdk/+/388822)
• implement in legacy protocol (https://dart-review.googlesource.com/c/sdk/+/388920)

[bwilkerson]

My feeling is that analysis_options.yaml should be priority ...

I agree. If a package specifies a default then I think most users would want that default to be honored more than they'd want their person preference to be used.

However, in the legacy protocol the value is passed to the individual edit.format call and so ignoring that value (because there's one in analysis_options) seems slightly weirder.

The only reason it's a request parameter is because we didn't provide any kind of configuration support. I don't have any concerns about treating the user preference the same across the IDEs, and I think it might be confusing if they were treated differently.

I harvested this code from analyzer initially, but ended up rewriting a lot because the YamlNode API is... weird.

I agree that the API is weird. We should consider adding an API to the analyzer package that will be performant enough for dart_style to be able to use it and for it to share code with the AnalysisContextCollection class' support for finding analysis options files. It would be bad if we had two ways of finding this information that diverged over time.

1. is this valid?

No.

Is 0 valid?

No, and neither are negative values, or any non-integer values. I'm not sure what a minimum valid value is (I assume that 1 is also not meaningful), or whether we should just accept any positive value and let the user get what they get.

We should treat an incomplete or invalid option as if it wasn't specified at all. In other words, they shouldn't change the default page width.

[DanTup]

The only reason it's a request parameter is because we didn't provide any kind of configuration support. I don't have any concerns about treating the user preference the same across the IDEs, and I think it might be confusing if they were treated differently.

Thanks - I'll do this. I think your other answers align with the assumptions I made the first CL at https://dart-review.googlesource.com/c/sdk/+/388822, though I am just accepting >= 1 because @munificent didn't mention a minimum - but I can update the validation if there is (if there is, perhaps it should be exposed by the formatter as a constant to avoid us duplicating it).

[DanTup]

This was done via 5d84439 and 75d15bb.

[munificent]

My feeling is that analysis_options.yaml should be priority, so if in VS Code you set your own default to 120 but clone a project using 100, you will get 100.

I agree. It's not good to be in a situation where your local preferences override a committed configuration because then any changes you make are likely to break CI or cause large diffs when you upload a PR.

However, in the legacy protocol the value is passed to the individual edit.format call and so ignoring that value (because there's one in analysis_options) seems slightly weirder.

I don't know what this means, but that's OK. :)

I presume the rules for overriding analysis_options.yaml in dart format are the same as what's been implemented for the server - eg. an analysis_options.yaml completely prevents use of values from any ancestor unless it was included?

Yes. I borrowed the implementation and semantics from server. A nearer analysis_options.yaml file always completely shadows an outer one, even if it doesn't have a particular setting.

I think these tests confirm that

Right.

although I couldn't see a test that uses include and specifically checks the pageWidth.

These tests are more like integration tests and validate that the page width makes its way from the analysis_options.yaml file all the way through to formatting.

1. is this valid?

formatter:
  page_width:

In the formatter's code for extracting a page_width from analysis_options.yaml, it silently fails if at any point it can't get to an integer for the page_width inside formatter. So in this case, I think YAML gives you null for the value? It should then silently reject that.

2. Is 0 valid?

It's definitely not going to do anything useful, but the formatter will let you try. :)

Thank you for the quick turnaround!

[DanTup]

2. Is 0 valid?

It's definitely not going to do anything useful, but the formatter will let you try. :)

I treated 0 as invalid, so if the user types this they will get a diagnostic (same as if they type a negative value), and we will ignore it (that is, we'll pass nothing to the formatter, or the globally configured value from the IDE).

I think this is fine, but if dart format would pass the zero to the formatter and you think we should do the same here for consistency, let me know. (I guess the same goes for negatives - are you treating them as entirely invalid and ignoring them and using 80, or would it do the same as 0/1?)

[munificent]

I think this is fine, but if dart format would pass the zero to the formatter and you think we should do the same here for consistency, let me know.

I honestly don't think it matters much one way or the other. For what it's worth, dart format will let you pass zero or a negative number and will then try to format at that (nonsensical width). I suspect that what you'll get is just maximally line split output.