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

add format option handlers #374

Merged
merged 3 commits into from
Oct 30, 2019
Merged

add format option handlers #374

merged 3 commits into from
Oct 30, 2019

Conversation

ZacLN
Copy link
Contributor

@ZacLN ZacLN commented Jul 22, 2019

Allows the passing of config options from client to DocumentFormat. Merging should be delayed until tagging of new DocFormat release + julia-vscode/julia-vscode#806 .

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #374 into master will increase coverage by 0.19%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
+ Coverage   33.14%   33.33%   +0.19%     
==========================================
  Files          22       22              
  Lines        1219     1230      +11     
==========================================
+ Hits          404      410       +6     
- Misses        815      820       +5
Impacted Files Coverage Δ
src/requests/features.jl 0% <0%> (ø) ⬆️
src/requests/workspace.jl 0% <0%> (ø) ⬆️
src/requests/init.jl 54.76% <100%> (+1.1%) ⬆️
src/languageserverinstance.jl 88.09% <40%> (-6.5%) ⬇️
src/protocol/configuration.jl 100% <0%> (ø) ⬆️
src/jsonrpc.jl 72.72% <0%> (+3.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f31248d...8f69ebd. Read the comment docs.

@davidanthoff davidanthoff added this to the v0.6.1 milestone Jul 23, 2019
@davidanthoff davidanthoff modified the milestones: v0.6.1, v0.7.0 Aug 13, 2019
@davidanthoff davidanthoff merged commit 2a71d80 into master Oct 30, 2019
@davidanthoff davidanthoff deleted the docformat branch October 30, 2019 19:03
@non-Jedi
Copy link
Member

non-Jedi commented Nov 5, 2019

This broke master for use with eglot. The server sends:

[ Info: SENT: {"method":"workspace/configuration","id":-100,"params":{"items":[{"scopeUri":null,"section":"julia.format.indent"},{"scopeUri":null,"section":"julia.format.indents"},{"scopeUri":null,"section":"julia.format.ops"},{"scopeUri":null,"section":"julia.format.tuples"},{"scopeUri":null,"section":"julia.format.curly"},{"scopeUri":null,"section":"julia.format.calls"},{"scopeUri":null,"section":"julia.format.iterOps"},{"scopeUri":null,"section":"julia.format.comments"},{"scopeUri":null,"section":"julia.format.docs"},{"scopeUri":null,"section":"julia.format.lineends"}]},"jsonrpc":"2.0"}

and eglot responds with

[ Info: RECEIVED: {"jsonrpc":"2.0","id":-100,"result":null,"error":null}

And of course length is not defined for nothing.

Reading the specification, it seems like eglot may be in the wrong here since the type of result is supposed to be any[], but I could be misinterpreting it. I'm not very familiar with the language they're using to describe the interfaces.

@non-Jedi
Copy link
Member

non-Jedi commented Nov 5, 2019

This PR doesn't follow the spec in that it sets scopeUri to nothing when it needs to be a valid DocumentUri (a string). Any chance of rolling this PR back and releasing a 1.0.1?

@davidanthoff
Copy link
Member

Could you try whether #416 fixes the problem?

If I understand the @json_read macro correctly, then it will lead to a situation where serializing a struct with an optional field will actually just not put that field into the JSON representation if the value is nothing, which is probably what we want here, right?

@non-Jedi
Copy link
Member

non-Jedi commented Nov 6, 2019

Ya, that would be what we want. I checked out 7330639 from #416, and I'm still seeing "scopeUri": null. Would you like me to open an issue for this, so it's tracked better than in the comments of an already merged PR?

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.

3 participants