Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use JuliaFormatter instead of DocumentFormat #972

Merged
merged 11 commits into from
Nov 4, 2021
Merged

Conversation

pfitzseb
Copy link
Member

@pfitzseb pfitzseb commented Aug 2, 2021

non-Jedi and others added 3 commits June 16, 2020 09:50
Closes julia-vscode/DocumentFormat.jl#70

JuliaFormatter was originally forked from DocumentFormat when domluna
wanted to add canonicalising functionality. 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.
@pfitzseb
Copy link
Member Author

pfitzseb commented Aug 2, 2021

Requires domluna/JuliaFormatter.jl#448 for compat with older Julia versions.

@pfitzseb
Copy link
Member Author

pfitzseb commented Aug 3, 2021

This now also adds some rudimentary support for selection formatting.

@pfitzseb pfitzseb marked this pull request as ready for review August 3, 2021 16:08
@davidanthoff davidanthoff added this to the Next Minor milestone Aug 23, 2021
Copy link
Member

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think the main blocker at the moment is that we're waiting for MinimalStyle, i.e. domluna/JuliaFormatter.jl#256, right?

@davidanthoff
Copy link
Member

Oh, and also, it would be great if @ZacLN could generally take a look at JuliaFormatter.jl etc and chime in.

longest_prefix = CSTParser.longest_common_prefix(something(longest_prefix, line), line)
end

config = get_juliaformatter_config(doc, server)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this request get hit every key stroke? If so it's not ideal to search the disk each time. I can't remember whether we're able to have a file watch from within the languageserver which may be why this has been done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just add those files to https://github.com/julia-vscode/julia-vscode/blob/master/src/extension.ts#L232, and then we would get normal LSP messages about their content etc. That seems in general better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this request get hit every key stroke?

No, only when explicitly requested.

We could just add those files to https://github.com/julia-vscode/julia-vscode/blob/master/src/extension.ts#L232, and then we would get normal LSP messages about their content etc. That seems in general better?

IIUC we'd only be notified by FS events or when the user opens/changes the .JuliaFormatter.toml. The client won't just dump all matching files into the server.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client won't just dump all matching files into the server.

Not sure what you mean by that. We could just handle .JuliaFormatter.toml files the same way we handle all .jl files in the workspace, right? Then they would be in-memory always?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that so? I thought we'd load most files directly from disk?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we load initially, but then we don't need to reload because we get notifications whenever they are updated. If this is not hit with every keystroke that probably doesn't matter that much, though.

Having said that, the LSP will have to move to a model at some point where language servers just don't touch the file system at all and instead can get all content from the client, just to make things work properly with virtual file systems that are supported in the client. At the moment there is not support for that in the protocol itself, but we would probably be better prepared for that if we made these files "normal" files. On the other hand, lets not make that a roadblock for this PR :) The MinimalStyle is, though...

@davidanthoff davidanthoff modified the milestones: Next Minor, Backlog Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration/merge with JuliaFormatter?
4 participants