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

expose deserialize convenience methods #257

Closed
wants to merge 9 commits into from

Conversation

mainrs
Copy link
Contributor

@mainrs mainrs commented Jul 13, 2020

Weirdly enough, the methods were already implemented but not exposed by the root module (ron). They could only be accessed by going through ron::{de, se}, which made me believe that they do not exist.

This PR adds them to the root module to make discovery easier.

Fixes #255.

@kvark
Copy link
Collaborator

kvark commented Jul 14, 2020

Thank you for the PR!
I'd like to have the exports consistent. If we are re-exporting a lot of stuff from the root, we should ask ourselves a question - do the sub-modules still need to have anything they publicly contain? Maybe instead, they should be private modules, and we'll re-export everything that needs to be accessible.

@mainrs
Copy link
Contributor Author

mainrs commented Jul 14, 2020

I re-exported them as they fit the way serde_json does it, which is, in my opinion, a good comparative crate after all :). Having all read/write functions in the root is just what I expect from using a serde crate. serde_json does keep the other moduls public as well.

I do not see any real benefit from not re-exporting the methods. Re-exporting them is similar to a prelude module found in larger crates like diesel for example.

src/lib.rs Show resolved Hide resolved
@mainrs
Copy link
Contributor Author

mainrs commented Jul 15, 2020

I've read and did some more research about serde and crates that provide data formats for it. I stumbled across the following document provided by serde: https://serde.rs/conventions.html

The layout should look like this in basic scenarios:

mod de;
mod error;
mod ser;

pub use de::{from_str, Deserializer};
pub use error::{Error, Result};
pub use ser::{to_string, Serializer};

According to this:

In addition, formats that provide serialization-specific or deserialization-specific APIs beyond Serializer and Deserializer should expose those under top-level ser and de modules.

The PrettyConfig module would fall under this category and should be provided by the de module inside the crate.

Additionally:

One or more to_abc functions depending on what types the format supports serializing to. For example to_string which returns a String, to_bytes which returns a Vec, or to_writer which writes into an io::Write.

This would apply for all to_xxx methods as well as from_xxx methods (see next point inside the document). Meaning that these should be exposed by the top-level crate instead of having to be imported.

Given that those are somewhat official API design and API guidelines by the people who created serde I'd suggest the following:

  • Expose from_bytes and from_reader in the top-level crate.
  • Expose to_string_pretty, to_writer in the top-level crate.
  • Do not make de and ser private.
  • Expose PrettyConfig through de, as it is a quote: deserialization-specific API.

@kvark
Copy link
Collaborator

kvark commented Jul 16, 2020

Thank you for writing this down!
I think we should still opt into making the ser and de modules private. It's just nicer this way. We'll expose PrettyConfig at the root.
Now, master branch is fine with breaking changes, this is the proper place for them to happen.

A backwards-compatible version of this PR would need to be made against v0.6 branch, with an extra commit that appends to the CHANGELOG and bumps the patch version. This is where we publish from.

@mainrs
Copy link
Contributor Author

mainrs commented Mar 21, 2021

I made se/de private as discussed above and exported all relevant types to the root of the crate. The change is a breaking change. The commits should probably be squashed when merged as I accidentally worked in another branch I had so the commit history is quite noisy.

@kvark
Copy link
Collaborator

kvark commented Mar 22, 2021

Please make the CI happy

@mainrs
Copy link
Contributor Author

mainrs commented Mar 22, 2021

Please make the CI happy

Should be done. However, tests/comments.rs might fail. It seems that it imports ErrorCode and Position, neither of which are exported inside the master branch's lib.rs file. I can export them, but I don't understand how they became part of the public API even though lib.rs didn't export them 🤔

@github-actions
Copy link

Issue has had no activity in the last 60 days and is going to be closed in 7 days if no further activity occurs

@github-actions github-actions bot added the stale label Nov 18, 2021
@github-actions github-actions bot closed this Nov 25, 2021
@kvark kvark reopened this Dec 3, 2021
@github-actions github-actions bot closed this Dec 11, 2021
@kvark
Copy link
Collaborator

kvark commented Dec 17, 2021

comment

@kvark kvark reopened this Dec 17, 2021
@github-actions github-actions bot removed the stale label Dec 18, 2021
@juntyr juntyr marked this pull request as draft January 28, 2022 17:25
@torkleyy
Copy link
Contributor

These methods are exposed now, so I'm closing this PR. Feel free to reopen if there's something I missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add utility methods from_reader, from_slice, from_value similar to serde_json
3 participants