RUST-447 Standardize API with other serde libraries #179

isabelatkinson · 2020-05-29T01:03:22Z

This PR updates the bson API language to use "serialize" and "deserialize" in place of "encode" and "decode" respectively.

isabelatkinson · 2020-05-29T01:20:47Z

src/de/serde.rs

    Deserialize,
    DeserializeSeed,
-    Deserializer,
+    Deserializer as SerdeDeserializer,


I think this rename needs to occur because the original name conflicts with the newly-named Deserialize struct, but the import needs to remain as-is for this line, but let me know if there's a better workaround.

Since you don't need to use the name SerdeDeserializer at all, I think you can actually use Deserializer as _, which will include it for the purposes of values that implement that trait but won't put any name into scope.

isabelatkinson · 2020-05-29T01:26:11Z

src/de/mod.rs

 };

-use ::serde::de::{Deserialize, Error};
+use ::serde::de::{self, Error as SerdeError};


Same conflict as other rename.

isabelatkinson · 2020-05-29T01:32:48Z

src/document.rs

-    /// Attempts to encode the `Document` into a byte stream.
-    pub fn encode<W: Write + ?Sized>(&self, writer: &mut W) -> EncoderResult<()> {
+    /// Attempts to serialize the `Document` into a byte stream.
+    pub fn serialize_doc<W: Write + ?Sized>(&self, writer: &mut W) -> crate::ser::Result<()> {


Since Document implements the Serialize trait here, and calls serialize here, the compiler wasn't happy with this method being named serialize. (I had no idea Rust doesn't allow method overloading, interesting.)

Good catch, I hadn't thought of this conflict!. Technically you could have two different methods with the same name if one of them is from a trait, but it's not traditional overloading because there's no disambiguation at the call site based on the parameters; you'd only have access to one of methods for a given variable based on what type it is. Here's an example of that: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=996a1b2bab9c7a8602de0098de00012c

I think I might prefer the name serialize_to here; when calling it, I think it would read well, e.g. doc.serialize_to(&mut writer)?.

I agree, less redundant than having doc in the name

I like serialize_to, but we might also want to consider something like to_writer and from_reader like in serde_json and serde-xml-rs.

I'm fine with from_reader and to_writer.

I'm fine with both, I'll leave as-is with serialize_to unless anyone feels super strongly either way.

You can change it to Patrick's suggestions; sorry if I was unclear

oops sorry I didn't see that first comment! I had this window sitting open for a while and didn't realize you need to refresh to see new comments. will do

No worries; github does require a lot of refreshes that it should probably trigger on its own

saghm · 2020-05-29T15:10:03Z

src/de/error.rs

 #[derive(Debug)]
 #[non_exhaustive]
-pub enum DecoderError {
+pub enum Error {


Since we have a separate error type in the de module than at the top level (as opposed to something like serde_json, which has only one error type in the entire crate`, I think we still might want to have a prefix in the name of this type (and for the Result type below); it's a bit friendlier to users who need to import multiple Error types from our crate if we give them unique names.

@patrickfreed is there precedence in other serde libraries for this naming style when there's more than one error/result defined? I agree that it's a little less user friendly to use something like crate::de::Error rather than DeserializerError.

I think it's pretty standard to rely on the module path for these kinds of things. All the serialization libraries that I can find which have separate errors for serialization / deserialization simply name each Error (toml, xml, serde_urlencoded), and serde itself also follows this pattern with its ser::Error and de::Error traits. I think we should keep to this pattern for consistency with other libraries.

That makes sense, I think consistency with serde would be good. @saghm what do you think?

I guess it makes sense to be consistent with our oid::Error. I'm not sure what you're referring to with the xml-rs crate, though; as it doesn't seem to have ser or de modules; do you mean serde_xml?

The xml crate I was referring to appears to actually just be a parsing library, not a serialization one. It has reader and writer modules with their own Errors which are somewhat analogous to de and ser. serde_xml-rs appears to be the actual corresponding serialization crate for xml.

saghm · 2020-05-29T15:19:28Z

src/document.rs

-    /// Attempts to encode the `Document` into a byte stream.
-    pub fn encode<W: Write + ?Sized>(&self, writer: &mut W) -> EncoderResult<()> {
+    /// Attempts to serialize the `Document` into a byte stream.
+    pub fn serialize_doc<W: Write + ?Sized>(&self, writer: &mut W) -> crate::ser::Result<()> {


Good catch, I hadn't thought of this conflict!. Technically you could have two different methods with the same name if one of them is from a trait, but it's not traditional overloading because there's no disambiguation at the call site based on the parameters; you'd only have access to one of methods for a given variable based on what type it is. Here's an example of that: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=996a1b2bab9c7a8602de0098de00012c

I think I might prefer the name serialize_to here; when calling it, I think it would read well, e.g. doc.serialize_to(&mut writer)?.

saghm · 2020-05-29T15:20:39Z

src/document.rs

-    /// Attempts to decode a `Document` from a byte stream.
-    pub fn decode<R: Read + ?Sized>(reader: &mut R) -> DecoderResult<Document> {
+    /// Attempts to deserialize a `Document` from a byte stream.
+    pub fn deserialize<R: Read + ?Sized>(reader: &mut R) -> crate::de::Result<Document> {


If you go with my serialize_to suggestion above, I think this should be renamed to deserialize_from for parity.

saghm · 2020-05-29T15:21:14Z

src/ser/error.rs

 #[derive(Debug)]
 #[non_exhaustive]
-pub enum EncoderError {
+pub enum Error {


Ditto above suggestion to prefer the name of the Error/Result types in this module

saghm · 2020-05-29T15:26:41Z

src/de/serde.rs

    Deserialize,
    DeserializeSeed,
-    Deserializer,
+    Deserializer as SerdeDeserializer,


Since you don't need to use the name SerdeDeserializer at all, I think you can actually use Deserializer as _, which will include it for the purposes of values that implement that trait but won't put any name into scope.

isabelatkinson commented May 29, 2020

View reviewed changes

isabelatkinson requested review from saghm and patrickfreed May 29, 2020 01:35

saghm reviewed May 29, 2020

View reviewed changes

isabelatkinson added 5 commits May 29, 2020 14:47

RUST-447 Standardize API with other serde libraries

baffdc6

formatting

98cbb6b

update prints

2180af6

restore ::

c08683e

update import rename and serialize/deserialize names

78a7834

isabelatkinson force-pushed the RUST-447 branch from 981057e to 78a7834 Compare May 29, 2020 20:20

rename document methods

c35492f

patrickfreed approved these changes May 30, 2020

View reviewed changes

saghm approved these changes Jun 1, 2020

View reviewed changes

isabelatkinson merged commit ccb6827 into mongodb:master Jun 1, 2020

isabelatkinson deleted the RUST-447 branch June 1, 2020 20:28

RUST-447 Standardize API with other serde libraries #179

RUST-447 Standardize API with other serde libraries #179

Uh oh!

Conversation

isabelatkinson commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickfreed May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isabelatkinson commented May 29, 2020 •

edited

Loading

patrickfreed May 29, 2020 •

edited

Loading