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

rename JSONRPCError field msg -> message #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ZacLN
Copy link

@ZacLN ZacLN commented May 31, 2020

msg was inconsistent with the JSONRPC spec and usage within the package (catch block of dispatch_msg).

Not clear if this requires downstream changes/version bump?

@ZacLN ZacLN added the bug Something isn't working label May 31, 2020
@ZacLN ZacLN requested a review from davidanthoff May 31, 2020 18:45
@ZacLN ZacLN added this to the Next extension patch release milestone May 31, 2020
@codecov
Copy link

codecov bot commented May 31, 2020

Codecov Report

Merging #18 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #18   +/-   ##
======================================
  Coverage    3.70%   3.70%           
======================================
  Files           4       4           
  Lines         135     135           
======================================
  Hits            5       5           
  Misses        130     130           
Flag Coverage Δ
#unittests 3.70% <0.00%> (ø)
Impacted Files Coverage Δ
src/core.jl 0.00% <0.00%> (ø)

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 b2b474b...70d6aff. Read the comment docs.

@ZacLN ZacLN self-assigned this May 31, 2020
@davidanthoff
Copy link
Member

On the other hand, the Julia exception types all use msg, that is why I used that here as well. Is there actually an accessor function in Julia that generically extracts the message from an exception type and abstracts away of the precise field names?

@ZacLN
Copy link
Author

ZacLN commented May 31, 2020

I'm referring to https://github.com/julia-vscode/JSONRPC.jl/blob/master/src/typed.jl#L71 rather than the within-julia error throwing. The receiver of that error message will expect to get a "message" entry won't they?

@davidanthoff
Copy link
Member

The code you link to is a bug, #16 is meant to fix that. We should probably merge that first before we make further changes.

@davidanthoff
Copy link
Member

Do you think we should still do this, or is the current master version fine?

@ZacLN
Copy link
Author

ZacLN commented Jun 2, 2020

It's mildly confusing, may as well change it

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.

Cool, sounds good.

I think it also need to be changed at https://github.com/julia-vscode/JSONRPC.jl/blob/master/src/typed.jl#L67.

I don't think any of the downstream packages need an update.

@davidanthoff
Copy link
Member

@ZacLN Bump, I think you need to make one more change and then we can merge this.

@davidanthoff davidanthoff modified the milestones: Next extension patch release, Backlog Jun 20, 2020
@davidanthoff davidanthoff removed this from the Backlog milestone Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants