Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

ack_responses #519

Merged
merged 3 commits into from
Oct 20, 2017
Merged

ack_responses #519

merged 3 commits into from
Oct 20, 2017

Conversation

philip-alldredge
Copy link
Contributor

This pull request fixes the RLS not sending response when result is empty such as when the result of type Ack. I came across this when experimenting with using the RLS in Eclipse via LSP4E/LSP4J. Those libraries wait for a response even if the result is expected to be empty or null. I'm an new at rust so this may be an inappropriate way to solve the problem.

Adds test to verify shutdown response is received.
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR! I think there might be a better way to fix this problem - I think that whether a response should be sent or not should be a property of the response type rather than property of the action. So, if we want to not send a response then the response type should be NoResponse (a new zero-sized type), rather than ().

I'm not sure exactly the best way to then send or not send a response, but probably the best way is to add a Response trait with a send method where there is a default implementation that calls out.success and NoResponse does nothing.

@nrc
Copy link
Member

nrc commented Oct 17, 2017

And a very minor point, could you take a look at your editor tab settings - there is a mix of tabs and spaces in the PR. The RLS uses four spaces for tabs.

@philip-alldredge
Copy link
Contributor Author

I have pushed the requested changes. However, the Response trait doesn't have a default implementation because it requires the type for which the train is implemented to be serializable so that out.success may be called.

@nrc
Copy link
Member

nrc commented Oct 20, 2017

This looks great, thanks for the changes!

@nrc nrc merged commit 78175cf into rust-lang:master Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants