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

Simplify outgoing notifications, remove NoParams #603

Merged
merged 2 commits into from
Nov 29, 2017

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Nov 28, 2017

Work towards #602. Since I'll want use different notifications (window/showMessage) than the currently hardcoded PublishDiagnostics and rustDocument/* ones, I tried to generalize the Output::notify, which is now easily possibly thanks to Action trait (thanks @alexheretic!).
The trait is generic enough (provides method and params type) to be easily reused, but it's not as clean as it could be - per documentation implementers are messages handled by the server (so client->server), whereas I added implementation of the ones sent to the client instead.

Ideally this should be solved using gluon-lang/lsp-types#19 (which I didn't do yet... 😢 ).

Additionally replaced NoParams with (), as these serve the same purpose and not to introduce another custom type.

r? @nrc

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.

First two commits look good, I'm not sure about removing NoParams - did this work OK?

{
Ok(NoParams)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The point in having NoParams is for the serialisation and deserialisation, I think that if you use () there are bugs both ways (serialised as () rather than {} or maybe ``, iirc). It's possible some other change means this is no longer an issue though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will be completely honest here - I'm guilty of relying solely on test suite for NoParams change. Now that I look at it they're not covering much messages with NoParams, except for test_shutdown, but that hands off the already structured Request.
I may have jumped gun on that one. I'll discard the last commit for now, thanks for double-checking this!

@Xanewok Xanewok force-pushed the simplify-out-notifications branch from d234212 to 09630fd Compare November 29, 2017 00:43
@Xanewok Xanewok force-pushed the simplify-out-notifications branch from 09630fd to 3f41009 Compare November 29, 2017 00:44
@Xanewok
Copy link
Member Author

Xanewok commented Nov 29, 2017

Rebased and force-pushed without the last NoParams -> () commit. As per #603 (comment), I'll revisit the last change when I have more tests.

@nrc nrc merged commit 41bf9af into rust-lang:master Nov 29, 2017
@nrc
Copy link
Member

nrc commented Nov 29, 2017

Thanks!

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