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

Refactor Error Code definitions #1270

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Seelengrab
Copy link
Contributor

This refactors the way error codes are handled, in preparation for making JSONRPC.jl less LSP specific, by hooking into custom error printing provided by an upstream PR. Requires julia-vscode/JSONRPC.jl#70.

This refactors the way error codes are handled, in preparation for
making JSONRPC.jl less LSP specific, by hooking into custom error printing
provided by an upstream PR.
@Seelengrab Seelengrab marked this pull request as draft February 25, 2024 10:32
@Seelengrab
Copy link
Contributor Author

Seelengrab commented Feb 25, 2024

There's also these error codes that seem to overlap considerably:

function textDocument_formatting_request(params::DocumentFormattingParams, server::LanguageServerInstance, conn)
doc = getdocument(server, params.textDocument.uri)
newcontent = try
config = get_juliaformatter_config(doc, server)
format_text(get_text(doc), params, config)
catch err
return JSONRPC.JSONRPCError(
-32000,
"Failed to format document: $err.",
nothing
)
end

This error code is seemingly used for a formatting failure, but the LSP spec says this specific code should not be used:

/**
 * This is the end range of JSON-RPC reserved error codes.
 * It doesn't denote a real error code.
 *
 * @since 3.16.0
 */
export const jsonrpcReservedErrorRangeEnd = -32000;

This should probably be changed to some other error code, but I don't know how that would interact with the extension.

Similarly, this section also defines a new error code for formatting:

# Format the full marked text
text_formatted = try
config = get_juliaformatter_config(doc, server)
format_text(text_marked, params, config)
catch err
return JSONRPC.JSONRPCError(
-33000,
"Failed to format document: $err.",
nothing
)
end

This doesn't conflict with the spec, but it doesn't match the existing formatting error code used above. This needs a decision on whether they should be merged into one error code, or kept distinct.

@pfitzseb
Copy link
Member

pfitzseb commented Mar 4, 2024

Both of those should be -33000 I think. As far as I'm aware no-one is actually checking for the specific error code anyways, FWIW.

Since this is basically the same errors, unify the two
error codes into one. Technically the two are subtly different
(one is trying to format the whole document, while the other
is only trying to format a section), but the underlying cause
is likely the same, so it's fine to merge them. If necessary/required,
they can be disentangled later on too, as currently noone seems to
check them anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants