Remove redundant serialization and deserialization in printer

[icsaszar]

Affects #128.

• Needs tests.

[ebkalderon]

Changes look good overall! Continuing the discussion from #128, I wonder what we should do about the out-of-order serialization of fields. It's not a problem to simply update the failing tests, in theory, but now I realize that the serialization order of request fields is now inconsistent between client-to-server messages and server-to-client messages, since they aren't relying on the same jsonrpc_core::types::params::Params implementation of Serialize anymore.

[ebkalderon]

An alternative approach I just thought of which still has a two-step serialization process, but reuses the upstream jsonrpc type for consistent field ordering, is much lighter weight than the status quo, and avoids the double string allocation is to use serde_json::to_value() and serde_json::from_value() to convert from an N::Params to a jsonrpc_core::types::params::Params.

[ebkalderon]

Okay, I've opened #131 which would unblock us from working on refactoring Printer and adding support for proper client requests. Let's review this PR first and then move on to that one, since you opened this first.

BTW, I also merged a tiny logging PR in the meantime, so I'm sorry for rendering your PR out of date. Would you mind kindly rebasing your PR against master? Feel free to ping me if you need anything.

[icsaszar]

@ebkalderon What do you think of testing the current (custom struct) implementation with deserialinzing the strings into maps and comparing them? I've pushed a commit that changes the tests to compare the parsed jsonrpc_core::Value.

But if you would prefer to use to_value and from_value instead of the current solution I'll make the change to that.

[ebkalderon]

Sorry for the delayed response, @icsaszar. I just got a little busy. Thank you very much for the PR!

It looks like the struct serialization technique works great! I also stress-tested the old and new serialization functions by changing out the request_serialization_ok() test to use Initialize and a fully populated InitializeParams instead, everything passed. 🎉 I think it should be safe to remove the make_request_old() and make_notification_old() functions completely.

I also left a few suggestions in the review comments below.

[ebkalderon]

Also, since #131 has been merged, please consider rebasing your PR against the latest master! The changes mostly affected src/delegate.rs and not src/delegate/printer.rs, so this should hopefully have no conflicts. As always, feel free to ping me if you run into issues. ☺️

After this PR gets merged, I'll get started on refactoring Printer a little further so Printer::apply_edit(), Printer::register_capability(), and Printer::unregister_capability() are all changed to be async fn and start work on the client request routing.

[icsaszar]

@ebkalderon I made the requested changes.

I think it should be safe to remove the make_request_old() and make_notification_old() functions completely.

I've also removed the tests that used these since without the reference values, there isn't much we can test.

[ebkalderon]

Looks terrific, thank you!