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

Remove rustc serialize support #95

Merged
merged 6 commits into from Jan 31, 2017
Merged

Remove rustc serialize support #95

merged 6 commits into from Jan 31, 2017

Conversation

@TyOverby
Copy link
Collaborator

TyOverby commented Jan 31, 2017

No description provided.

src/lib.rs Outdated
#[cfg(feature = "rustc-serialize")]
pub mod rustc_serialize;
#[cfg(feature = "serde")]
pub mod refbox;
pub mod serde;

This comment has been minimized.

@dtolnay

dtolnay Jan 31, 2017

Collaborator

Let's make this a private module and pub use serde::*. We no longer need the namespacing.

This comment has been minimized.

@aldanor

aldanor Jan 31, 2017

Just wanted to say the same thing, +1

This comment has been minimized.

@TyOverby

TyOverby Jan 31, 2017

Author Collaborator

👍

@dtolnay
Copy link
Collaborator

dtolnay commented Jan 31, 2017

My thoughts on the public API after browsing https://docs.rs/bincode/1.0.0-alpha1/bincode/:

  • I would prefer to merge SerializeError and DeserializeError into bincode::Error and similarly SerializeResult and DeserializeResult into bincode::Result<T>. Nobody cares about the difference between these.
  • from_slice and from_reader would be more idiomatic than deserialize and deserialize_from, and to_vec and to_writer rather than serialize and serialize_into.
  • Pay attention to ?Sized - the io::Read and io::Write type parameters can be ?Sized, as can the &T where T: Serialize.
@TyOverby
Copy link
Collaborator Author

TyOverby commented Jan 31, 2017

I would prefer to merge SerializeError and DeserializeError into bincode::Error and similarly SerializeResult and DeserializeResult into bincode::Result. Nobody cares about the difference between these.

Considering how close these enums are, I think this is totally reasonable.

from_slice and from_reader would be more idiomatic than deserialize and deserialize_from, and to_vec and to_writer rather than serialize and serialize_into.

I guess if people are prefixing (e.g. bincode::from_slice) then this has readability wins, but I think that seeing a call to to_vec in the middle of a huge block of code (without knowing what is being imported) could be pretty confusing. Meanwhile, serialize is pretty descriptive.

Pay attention to ?Sized - the io::Read and io::Write type parameters can be ?Sized, as can the &T where T: Serialize.

Could you go more into detail here? I'm not really sure what it is that you're warning about.

@ZoeyR
Copy link
Collaborator

ZoeyR commented Jan 31, 2017

In regards to what @dtolnay is talking about. functions like bincode::serde::serialize_into put unnecessary restrictions on their type parameters. Both the W and T should be ?Sized.

@dtolnay
Copy link
Collaborator

dtolnay commented Jan 31, 2017

Could you go more into detail here? I'm not really sure what it is that you're warning about.

Sure! Accepting a ?Sized type parameter for io::Read and io::Write means that it will work with &mut io::Read and &mut io::Write trait objects which are dynamically sized.

use std::fs::File;
use std::io::Read;

fn main() {
    let mut data = File::open("important.bin").unwrap();
    let trait_object: &mut Read = &mut data;
    deserialize_from(trait_object); // requires ?Sized
}

fn deserialize_from<R>(_: &mut R) where R: Read {}

And accepting a ?Sized type parameter for &T means that it can work with dynamically sized implementations of Serialize, which includes str and [T] among others.

fn main() {
    serialize("&str"); // requires ?Sized
}

trait Serialize {}
impl Serialize for str {}

fn serialize<T>(_: &T) where T: Serialize {}
@dtolnay
Copy link
Collaborator

dtolnay commented Jan 31, 2017

I filed #99 and #100 to follow up on the remaining issues in case you don't want to include them in this PR.

@TyOverby TyOverby force-pushed the remove-rustc-serialize branch from e53a137 to 1d04836 Jan 31, 2017
@TyOverby TyOverby merged commit ff3dab8 into master Jan 31, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@TyOverby TyOverby deleted the remove-rustc-serialize branch Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.