Skip to content

Move payload deserialisation out of handlers #53

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

Conversation

tintoy
Copy link
Collaborator

@tintoy tintoy commented Dec 19, 2017

This moves the deserialisation of payloads out of handlers and into the calling code (LspDispatcher).

This is so the common JsonSerializerSettings can be used by the calling
code.

NOTE: This code is currently non-functional - handlers have been
modified to take an Object instead of a JObject but the calling code
does not yet perform deserialisation.
@tintoy
Copy link
Collaborator Author

tintoy commented Dec 19, 2017

I'm wondering whether BodyType should be called PayloadType. What do you reckon?

@codecov
Copy link

codecov bot commented Dec 19, 2017

Codecov Report

Merging #53 into refactor/protocol will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           refactor/protocol      #53     +/-   ##
====================================================
- Coverage              70.11%   70.02%   -0.1%     
====================================================
  Files                    211      211             
  Lines                   2202     2202             
====================================================
- Hits                    1544     1542      -2     
- Misses                   658      660      +2
Impacted Files Coverage Δ
src/JsonRpc/ProcessScheduler.cs 87.27% <0%> (-3.64%) ⬇️

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 c601b27...768e63f. Read the comment docs.

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

I learned a long time ago to not fight about naming, 🚲🏠 (bike shedding) goes to far some times.

I think PayloadType makes more sense, but either name works for me.

@tintoy
Copy link
Collaborator Author

tintoy commented Dec 19, 2017

Ok - I'm going to change it then 😁

@tintoy
Copy link
Collaborator Author

tintoy commented Dec 19, 2017

Right, have renamed it and added a couple of tests :)

Providing CI checks pass, would you like me to add this work to your WIP branch?

@tintoy
Copy link
Collaborator Author

tintoy commented Dec 19, 2017

Ok we're good to go :)

@tintoy
Copy link
Collaborator Author

tintoy commented Jan 4, 2018

@david-driscoll - I'm going to merge this into your refactor/protocol branch so we can discuss any further work required in one place (#51), hope you're OK with that 😁

@tintoy tintoy merged commit 69f9d6b into OmniSharp:refactor/protocol Jan 4, 2018
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