Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow other applications to hook into showerror #70

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented Oct 17, 2022

Closes #68.

This change allows other applications to customize showerror(::IO, ::JSONRPCError) depending on their specific needs and chosen meanings for their errors. It also introduces the fixed error codes as constants, to allow them to be transparently referenced in other places of the codebase.

Regarding serverErrorStart and serverErrorEnd - I don't know exactly how they're used in the VSCode extension/LS.jl (are the messages themselves matched anywhere?), so these could be added explicitly until those dependents are explicitly adding them to RPCErrorStrings. I've added them for now to make tests pass, but having them implicit seems cleaner to me.

For every PR, please check the following:

@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.65%. Comparing base (5adc04b) to head (962d276).
Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   91.46%   89.65%   -1.81%     
==========================================
  Files           4        4              
  Lines         246      232      -14     
==========================================
- Hits          225      208      -17     
- Misses         21       24       +3     
Flag Coverage Δ
unittests 89.65% <100.00%> (-1.81%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Seelengrab
Copy link
Contributor Author

Not quite sure what the coverage diff is about - it seems to be inconsistent in terms of what it compares? Or I'm clicking on the wrong thing, not sure.

These were either not errors at all (just range definitions) or
were in the application specific range, so not appropriate to keep
in a more general library.
@Seelengrab
Copy link
Contributor Author

Seelengrab commented Mar 4, 2024

The original intention here was to keep the LSP specific ones around until the time that LanguageServer.jl can/wants to integrate the functionality, but since that would require compat bumping sooner or later anyway, might as well just remove them now and do a lockstep release with appropriate compat upper bounds and/or upper bounding previous versions in the registry.

The question is just - is this a technically breaking release, since it changes the error message of LanguageServer.jl from e.g. ContentModified to Unknown if JSONRPC.jl is held back? A version of LanguageServer.jl making use of this functionality definitely needs lower bound on the new version (presumably 1.4, since this is a new feature).

@pfitzseb
Copy link
Member

pfitzseb commented Mar 4, 2024

The question is just - is this a technically breaking release, since it changes the error message of LanguageServer.jl from e.g. ContentModified to Unknown if JSONRPC.jl is held back?

Mh, not sure. The error type doesn't change after all. I don't mind tagging a 2.0 for correctness either way, but imho that wouldn't really be necessary.

These were leftover from a previous version were the equal signs
were aligned vertically. Since these constants have their own
docstrings now, this is no longer necessary.
@Seelengrab
Copy link
Contributor Author

Mh, not sure. The error type doesn't change after all. I don't mind tagging a 2.0 for correctness either way, but imho that wouldn't really be necessary.

Yeah, I don't mind either way :) Whichever is more convenient to have this percolate through the dependencies without causing too much disruption.

Is there some NEWS file or release notes specific to JSONRPC.jl that this should be mentioned in as a new feature?

@pfitzseb pfitzseb merged commit 2079986 into julia-vscode:master Mar 5, 2024
46 of 47 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove LSP specific error codes
2 participants