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

Requests don't need Deserialize, responses don't need Serialize #9

Open
laanwj opened this issue Sep 6, 2018 · 3 comments
Open

Requests don't need Deserialize, responses don't need Serialize #9

laanwj opened this issue Sep 6, 2018 · 3 comments
Labels
🍼 good first issue Good for newcomers

Comments

@laanwj
Copy link
Owner

laanwj commented Sep 6, 2018

This is only a client-side library, so I think there's no need for requests to do anything with deserialization, and response to do anything with serialization. This is simply dead code.

Unless there's a strong case for keeping them—which I don't think, but please let me know—it would be good to remove them.

@laanwj laanwj added the 🍼 good first issue Good for newcomers label Sep 6, 2018
@Kixunil
Copy link
Contributor

Kixunil commented Sep 7, 2018

I developed my own pattern for exactly this case. I put protocol related types into a separate crate with two feature flags: server and client. Then I implement server side by depending on that crate with server feature enabled and same thing for the client.

It's probably overkill to use separate crate in this case, since the server isn't implemented in Rust. Using feature flag to enable server side might still be useful for testing, don't you think?

@laanwj
Copy link
Owner Author

laanwj commented Sep 7, 2018

Using feature flag to enable server side might still be useful for testing, don't you think?

I'm not convinced that it is even useful for testing. I considered it, but there is no point in testing round-trips if half of the functionality is not required. Can just as well simply compare the result.

The most useful testing work at this point would be #11, as it would detect incompatibilities between the actual lightningd and this crate.

@Kixunil
Copy link
Contributor

Kixunil commented Sep 8, 2018

Good point, just get rid of it and if anyone needs it in the future, it's ~three lines of code anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍼 good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants