RFC: Use JuliaFormatter instead of DocumentFormat #759

non-Jedi · 2020-06-16T14:08:30Z

Closes julia-vscode/DocumentFormat.jl#70

JuliaFormatter was originally forked from DocumentFormat when @domluna
wanted to add canonicalising functionality. This resulted in a
large-scale refactoring that could not be immediately merged to
DocumentFormat (julia-vscode/DocumentFormat.jl#27).
Since then, extensive work has gone into JuliaFormatter. I believe it is
significantly more feature-rich than DocumentFormat at this time.

I believe there are significant advantages both to the Julia community
at large and to LanguageServer.jl to combine efforts on a single code
formatter.

Possible objections (I'm updating this list as discussions continue):

Missing features from DocumentFormat not present in JuliaFormatter.
- I am not aware of any but this should be more thoroughly reviewed.
JuliaFormatter is more aggressive than desired (e.g. by inserting line breaks).
- My understanding is that this could be mitigated using JuliaFormatter's style system. Doing so would require more work before merging this.
- WIP DocumentFormat.jl style at Style similar to DocumentFormat.jl domluna/JuliaFormatter.jl#256
Lack of julia-vscode governance.
- Taking a dependency on something not controlled by julia-vscode members may not be desired for reasons of making sure necessary features and fixes for vscode releases are merged in a timely manner.
JuliaFormatter has an additional dependency on DataStructures. My understanding is that this is undesirable for the way the vscode extension is packaged.
Stability concerns (crashing) with JuliaFormatter vs DocumentFormat

domluna · 2020-06-16T14:13:11Z

I'd like to point out a style that's very similar to what DocumentFormat currently does is possible. I just haven't gotten around to implementing it as of yet.

…

On Tue, Jun 16, 2020 at 10:08 AM Adam B ***@***.***> wrote: Closes julia-vscode/DocumentFormat.jl#70 <julia-vscode/DocumentFormat.jl#70> JuliaFormatter was originally forked from DocumentFormat when @domluna <https://github.com/domluna> wanted to add canonicalising functionality. This resulted in a large-scale refactoring that could not be immediately merged to DocumentFormat (julia-vscode/DocumentFormat.jl#27 <julia-vscode/DocumentFormat.jl#27>). Since then, extensive work has gone into JuliaFormatter. I believe it is significantly more feature-rich than DocumentFormat at this time. I believe there are significant advantages both to the Julia community at large and to LanguageServer.jl to combine efforts on a single code formatter. Possible objections: - Missing features from DocumentFormat not present in JuliaFormatter. - I am not aware of any but this should be more thoroughly reviewed. - JuliaFormatter is more aggressive than desired (e.g. by inserting line breaks). - My understanding is that this could be mitigated using JuliaFormatter's style system. Doing so would require more work before merging this. - Lack of julia-vscode governance. - Taking a dependency on something not controlled by julia-vscode members may not be desired for reasons of making sure necessary features and fixes for vscode releases are merged in a timely manner. - JuliaFormatter has an additional dependency on DataStructures. My understanding is that this is undesirable for the way the vscode extension is packaged. ------------------------------ You can view, comment on, or merge this pull request online at: #759 Commit Summary - Use JuliaFormatter instead of DocumentFormat File Changes - *M* Project.toml <https://github.com/julia-vscode/LanguageServer.jl/pull/759/files#diff-910a7b3ca0075d545dcec45cb7335ca9> (4) - *M* src/LanguageServer.jl <https://github.com/julia-vscode/LanguageServer.jl/pull/759/files#diff-e251ddbed724e1d3fadfada27af219bf> (3) - *A* src/formatoptions.jl <https://github.com/julia-vscode/LanguageServer.jl/pull/759/files#diff-cb2257acc06aa5b099254c4140ad67b2> (16) - *M* src/languageserverinstance.jl <https://github.com/julia-vscode/LanguageServer.jl/pull/759/files#diff-56cb1dd51eeed99d46e91753bd5b908b> (4) - *M* src/requests/features.jl <https://github.com/julia-vscode/LanguageServer.jl/pull/759/files#diff-d3bf537273c0143637e09498d5340088> (10) - *M* src/requests/workspace.jl <https://github.com/julia-vscode/LanguageServer.jl/pull/759/files#diff-8c94e46b3b83588f258c46a81780e5ab> (26) Patch Links: - https://github.com/julia-vscode/LanguageServer.jl/pull/759.patch - https://github.com/julia-vscode/LanguageServer.jl/pull/759.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#759>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAORUF3LNITQLHAQD7WSXVDRW54G5ANCNFSM4N7U7GFQ> .

non-Jedi · 2020-06-16T14:18:20Z

Thanks for chiming in @domluna, if you could point out which methods would need to be defined for a custom style in order to prevent inserting line breaks (since that was a point of concern in julia-vscode/DocumentFormat.jl#70), that would probably be helpful for this discussion. Or if it's more or less all of them, that's also valuable information. Is it as simple as defining nest_if_over_margin! to be a no-op?

pfitzseb · 2020-06-16T15:28:43Z

I'm in favour of this.

David has some concerns about this though. AFAICT JuliaFormatter is more widely used and more powerful than DocumentFormat, so imho it's a good idea regardless.

non-Jedi · 2020-06-16T15:42:06Z

I've added that concern to the OP.

My opinion is that even if there are regressions due to this change, JuliaFormatter provides enough additional features that making the change is worthwhile. Just in the past couple hours of using this branch, I've found myself actually making use of the formatting feature--which I had previously largely ignored--because it's so much better. As a practical example of the improvement, JuliaFormatter correctly formats #739 which is currently extremely difficult to read.

I believe getting DocumentFormat to have the same level of features as JuliaFormatter would introduce more bugs than currently exist in JuliaFormatter. If minimizing the absolute number of bugs in the codebase is the end goal, making this switch is counterproductive (as is introducing features to DocumentFormat), but if maximizing the utility to end-users is the goal, using JuliaFormatter is a better path to improving formatting than incrementally improving DocumentFormat.

aviatesk · 2020-06-16T15:50:23Z

src/formatoptions.jl

+const default_format_options = (4, 100)
+
+struct FormatOptions <: JuliaFormatter.AbstractStyle
+    indent::Int
+    margin::Int
+end
+FormatOptions() = FormatOptions(default_format_options...)
+
+FormatOptions(options::Vararg{Any,length(default_format_options)}) =
+    FormatOptions(something.(options, default_format_options)...)
+
+JuliaFormatter.getstyle(x::FormatOptions) = x
+
+# All functions that don't have a dispatch defined for FormatOptions
+# fallback to the definition for JuliaFormatter.DefaultStyle. This is
+# how we customize the behavior of the formatter.


I would like to first make sure whether we want to go with JuliaFormatter.jl before giving actual review, but here I also want to comment a bit about configurations here:
Actually with the .JuliaFormatter.toml configuration file, we don't need to handle specific formatting options by ourselves -- all what we need to do is to search for a configuration file and pass those configurations there like Juno does.
Then we get free from tasks to sync these frontend formatting option specs with actual formatter options anymore (in other word we won't need something like this PR.
So I think there won't be any overhead of syncing the development in frontend (i.e. julia-vscode side) and that of JuliaFormatter.jl once we setup the configuration handlings.

Yup, a config file is also how other prominent linters (e.g. ESLint) work and much more sensible than putting a bunch of options into every editor's UI.

I think it's still valuable to provide configuration at the editor level so that formatting files always works as the user expects without having to create a .JuliaFormatter.toml file for one-off scripts (if e.g. the user prefers 2-space tabs as used in the DifferentialEquations codebase). Ideally the editor should have a single option that sets this kind of stuff both for the editor's builtin indentation engine and the formatter. I agree that we should hook this up so that a .JuliaFormatter.toml file overrides language server configuration.

Yes, but imho that should only happen for "common" options like indentation (i.e. options that are shared by all languages).

How about this: we make the VS Code config settings scope application, so they can only be configured in user settings, but not as workspace or folder settings. I think what we should try to avoid is to have two different ways to configure folder specific settings, and I think the separate .JuliaFormatter.toml story then makes more sense than a VS Code specific configuration setting with window scope.

I think in terms of implementations this would mean that a .JuliaFormatter.toml would always take precedence (if it exists) over any config settings that the LS receives from the client, right?

What is a lot less clear to me is which config settings we should have in the client... Minimally I guess the option to specify a style? But I don't really see a good reason not to allow all the other settings to be specified as well...

domluna · 2020-06-16T16:51:33Z

Thanks for chiming in @domluna, if you could point out which methods would need to be defined for a custom style in order to prevent inserting line breaks (since that was a point of concern in julia-vscode/DocumentFormat.jl#70), that would probably be helpful for this discussion. Or if it's more or less all of them, that's also valuable information. Is it as simple as defining nest_if_over_margin! to be a no-op?

Sort of, the main thing is to make check whether the text is on the same line in the source file and set join_lines based on that.

https://github.com/domluna/JuliaFormatter.jl/blob/master/src/styles/yas.jl#L72

For example, in the above function, for tuples, everything is put on the same line. Which is why join_lines is set to true.

Also don't insert placeholder nodes, if those aren't there the nesting stage is a noop.

Of course there should be still be indentation of some sort, the YAS style indentation seems to be the closest to DocumentFormat but that could be explored.

I think it should be fairly straightforward to implement.

domluna · 2020-07-02T17:10:04Z

I have a WIP DocumentFormat-esque style domluna/JuliaFormatter.jl#256

ZacLN · 2020-07-02T18:24:17Z

Thanks @non-Jedi for the really helpful issue. Broadly I'm in support of this, going through your list:

(I'll add I've not actually used JF so please forgive any misconceptions)

Missing features. I assume JF is far ahead DF as is. My only question - Is JF currently able to apply formatting on the fly (i.e. on-keystroke) or is it structured so that this would be possible (and performant)? Leaving that option open was (I think) one of the drivers when designing DF.
This is my only concern. I don't like aggressive formatting, other do - there need to be options. I'd also suggest that they should fit into the vscode configuration story (including the option within vscode to point to whatever format of config file JF uses).
Not concernd about this as long as 2. and 5. are resolved in a satisfactory manner. I minimally maintain DF and really don't have much interest in it.
I don't have as a strong feeling on this as others but wouldn't seem to be a deal-breaker to me.
I agree with the points made by David on the other thread, but as a risk it would seem to be manageable. A bit dependent on whether @domluna is interested/able to commit a little bit of time to it.

domluna · 2020-07-02T20:22:39Z

My only question - Is JF currently able to apply formatting on the fly (i.e. on-keystroke) or is it structured so that this would be possible (and performant)? Leaving that option open was (I think) one of the drivers when designing DF.

The most common use of JF is formatting the entire file on save. You can pass arbitrary julia code and it'll format it but without the context of the entire file this might not be ideal. However, this might be sufficient for this use case but I'm not familiar with the API requirements.

Timings formatting for a julia file read into REPL s:

julia> @btime DocumentFormat.format($s);
  18.235 ms (54518 allocations: 4.20 MiB)

julia> @btime JuliaFormatter.format_text($s);
  3.088 ms (47567 allocations: 2.85 MiB)

Timings for a small piece of text, I imagine this would be a touch type use case (?)

julia> str = "foo(argument1,                           argument2,                                                    argument3            )"
"foo(argument1,                           argument2,                                                    argument3            )"

julia> @btime DocumentFormat.format($str);
  74.314 μs (256 allocations: 15.69 KiB)

julia> @btime JuliaFormatter.format_text($str);
  8.825 μs (230 allocations: 16.06 KiB)

using DocumentFormat v3.1.0

ZacLN · 2020-07-02T21:18:29Z

That looks good (in terms of being much faster) - I was kind of asking whether it is structured so that, for example, given some small node of a file's CST you could quickly get the context necessary to format that node without also formatting the whole file

EricForgy · 2020-07-02T21:43:42Z

I've never used JuliaFormatter, but this PR makes sense to me. I came across it because I wanted no spaces with TypeVar definitions.

Lack of julia-vscode governance.

Why not just move JuliaFormatter to an org? Maybe here?

domluna · 2020-07-02T23:10:57Z

That looks good (in terms of being much faster) - I was kind of asking whether it is structured so that, for example, given some small node of a file's CST you could quickly get the context necessary to format that node without also formatting the whole file

Partial formatting would probably be what you're looking for. This would still operate on the entire file (to figure out indentation and what not) but the output would only differ for the lines specified. So the lines that you don't want formatted would essentially have formatting disabled https://domluna.github.io/JuliaFormatter.jl/stable/skipping_formatting/

singularitti · 2020-07-03T02:44:08Z

Don't want to diverge here but I would love to see VSCode uses JF. I currently maintain Julia Formatter and it has a major issue: speed. The reason is I need to start and stop a Julia process for each file to be formatted. This formatter is the only one I have used to be waiting for seconds until it's done. It would be faster, I think, if you have a running Julia process all the time and have JF precompiled.

aviatesk · 2020-07-03T17:40:01Z

That looks good (in terms of being much faster) - I was kind of asking whether it is structured so that, for example, given some small node of a file's CST you could quickly get the context necessary to format that node without also formatting the whole file

AFAICT, JuliaFormatter works as far as an input text represents a valid Julia expressions and is parsable by CSTParser (am I right, @domluna ?).
So as an editor integration, we can support "selection-based formatting", which I think it covers @ZacLN 's concern (as a result, Juno supports 1.) whole document formatting 2.) selection-based formatting, and 3.) on-save (whole-document) formatting).

Another note, I found DocumentFormat works even if a given text doesn't represent a valid Julia expression, and I guess this is the main difference in term of how formatting works in both pkgs.

domluna · 2020-07-03T18:54:41Z

AFAICT, JuliaFormatter works as far as an input text represents a valid Julia expressions and is parsable by CSTParser (am I right, @domluna ?)

yeah that's right

non-Jedi · 2020-07-06T12:42:43Z

@ZacLN for mitigating point 5, would it be desirable to have usage of JuliaFormatter be opt-in via a configuration option for a release or two? Since DocumentFormat is really only called in one place, this wouldn't be difficult to accomplish.

jlperla · 2020-08-07T02:30:37Z

Not to be "that guy" who bumps PRs without contributing code, but...

Any hope on getting these sorts of features into vscode /either directly in the existing formatter or adopting this one?

fix for jmd parsing

…rrentBlockRange improve getCurrentBlockRange

backlog: add getDocFromWord handler

Enable cloud download

Don't duplicate completions

add rd/prepareRename

…-setting make symbolcache downloads configurable

because these are already indicated by highlighting the matching paren anyways

davidanthoff · 2021-07-24T03:30:23Z

I think the main blocker is that a) we're still not done with the major other work happening in LS land, and I'd rather have us finish us those parts first before we start a new big change, but more generally b) that no one had time for much lately :) But maybe we should just say that after Juliacon we make this here a priority.

…ect_file-return Fix env_project_file usage

pfitzseb · 2021-07-26T15:12:32Z

JuliaFormatter will also currently bring in 7 new dependencies, which isn't too great (good news is that there don't appear to be any binary dependencies, so distribution should just work).

…all-hover remove hover message for closing function call parenthesis

…for_root additional error handling in get_env_for_root

…Server.jl into non-Jedi-juliaformatter

pfitzseb · 2021-08-02T09:35:46Z

Ok, I have no idea what GitHub (or I) did with the commit history here. Will close this in favor of #972.

non-Jedi requested a review from davidanthoff June 16, 2020 14:10

non-Jedi added the enhancement label Jun 16, 2020

non-Jedi added this to the Triage milestone Jun 16, 2020

aviatesk reviewed Jun 16, 2020

View reviewed changes

domluna mentioned this pull request Sep 9, 2020

run JuliaFormmater fonsp/Pluto.jl#369

Closed

aviatesk mentioned this pull request Oct 30, 2020

Format file corrupts files julia-vscode/julia-vscode#1725

Closed

pfitzseb and others added 8 commits January 15, 2021 09:29

Merge pull request julia-vscode#859 from julia-vscode/jmdfix

6d82164

fix for jmd parsing

improve getCurrentBlockRange

17982b0

Merge branch 'master' into sp/better-getCurrentBlockRange

6293eaa

Merge pull request julia-vscode#860 from julia-vscode/sp/better-getCu…

3b5ddb3

…rrentBlockRange improve getCurrentBlockRange

Merge branch 'master' into avi/getdocfromword

5a5fec9

approx matches for search

e3c025a

Merge pull request julia-vscode#810 from julia-vscode/avi/getdocfromword

06aed7a

backlog: add getDocFromWord handler

don't store doc path in .val

f8b5dd9

ZacLN and others added 14 commits July 14, 2021 12:13

fix test

50cec5f

check prepareRename support

5380cbf

test fix

b47543d

Enable cloud download

2c86015

Fix indenting

8a0e8d3

Merge pull request julia-vscode#965 from julia-vscode/cloud-download

38514bb

Enable cloud download

Fix small bug

ebdc883

Merge branch 'master' into import-comps

f4e2888

Merge pull request julia-vscode#962 from julia-vscode/import-comps

2d941ac

Don't duplicate completions

Merge branch 'master' into preparerename

00a40a6

Merge pull request julia-vscode#963 from julia-vscode/preparerename

287be7f

add rd/prepareRename

make symbolcache downloads configurable

4504b14

Merge pull request julia-vscode#966 from julia-vscode/sp/ssc-download…

3093d16

…-setting make symbolcache downloads configurable

remove hover message for closing function call parenthesis

d962519

because these are already indicated by highlighting the matching paren anyways

davidanthoff and others added 4 commits July 25, 2021 11:00

Fix env_project_file usage

1d3e7d5

Merge pull request julia-vscode#969 from julia-vscode/handle-env_proj…

69a2088

…ect_file-return Fix env_project_file usage

Merge branch 'master' into sp/no-closing-call-hover

880757b

fix tests

477d703

davidanthoff and others added 8 commits July 26, 2021 09:17

Merge pull request julia-vscode#967 from julia-vscode/sp/no-closing-c…

38c1ee8

…all-hover remove hover message for closing function call parenthesis

additional error handling in get_env_for_root

274062b

fix showMessageRequest

747d4ac

Merge pull request julia-vscode#970 from julia-vscode/sp/fix-get_env_…

5e52361

…for_root additional error handling in get_env_for_root

Set version to v4.1.0

b855f13

Set version to v4.1.1-DEV

b8964d0

Merge branch 'juliaformatter' of https://github.com/non-Jedi/Language…

20d640e

…Server.jl into non-Jedi-juliaformatter

update

ae37ebf

pfitzseb closed this Aug 2, 2021

RFC: Use JuliaFormatter instead of DocumentFormat #759

RFC: Use JuliaFormatter instead of DocumentFormat #759

Uh oh!

Conversation

non-Jedi commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domluna commented Jun 16, 2020 via email

Uh oh!

non-Jedi commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfitzseb commented Jun 16, 2020

Uh oh!

non-Jedi commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviatesk Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfitzseb Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

non-Jedi Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfitzseb Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

davidanthoff Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

domluna commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domluna commented Jul 2, 2020

Uh oh!

ZacLN commented Jul 2, 2020

Uh oh!

domluna commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZacLN commented Jul 2, 2020

Uh oh!

EricForgy commented Jul 2, 2020

Uh oh!

domluna commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singularitti commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviatesk commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domluna commented Jul 3, 2020

Uh oh!

non-Jedi commented Jul 6, 2020

Uh oh!

jlperla commented Aug 7, 2020

Uh oh!

davidanthoff commented Jul 24, 2021

Uh oh!

pfitzseb commented Jul 26, 2021

Uh oh!

pfitzseb commented Aug 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

non-Jedi commented Jun 16, 2020 •

edited

Loading

non-Jedi commented Jun 16, 2020 •

edited

Loading

non-Jedi commented Jun 16, 2020 •

edited

Loading

aviatesk Jun 16, 2020 •

edited

Loading

non-Jedi Jun 16, 2020 •

edited

Loading

domluna commented Jun 16, 2020 •

edited

Loading

domluna commented Jul 2, 2020 •

edited

Loading

domluna commented Jul 2, 2020 •

edited

Loading

singularitti commented Jul 3, 2020 •

edited

Loading

aviatesk commented Jul 3, 2020 •

edited

Loading