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

Adding nightly serde support for VecMap under "eders" feature flag #15

Merged
merged 1 commit into from
Feb 15, 2016
Merged

Conversation

anp
Copy link
Contributor

@anp anp commented Feb 10, 2016

I'm working on adding serde support for rust-bio because many of the data structures used for bioinformatics can get quite large (many GB), and recomputing them takes a long time. rust-bio currently uses vec_map as a dependency for one of its index types, and I can't add serialization/deserialization to that type with serde without VecMap having it too.

This PR adds optional (under a feature flag) serde support for VecMap. Is this something you'd consider incorporating into the crate? The feature flag approach is how the maintainer of rust-bio preferred the implementation, so that's what I've used here to avoid using the buildscript approach that's usable on stable. My only gripe with this method is that I don't want to overload the nightly feature flag vec_map already uses, and I can't have a feature with the same name as a crate it depends on, so I chose the crate name's reverse. If this support is something you'd like to merge but would prefer a different feature flag (or would prefer to use a buildscript for the optional support), I'm more than happy to make any changes.

For context on the process of adding serde to rust-bio:

rust-bio/rust-bio#33
rust-bio/rust-bio#34

Also, as a more general question, I see that the same org owns the bit-set and bit-vec repos which are both dependencies in other rust-bio data structures, would you be willing to merge similar support for those as well?

@anp
Copy link
Contributor Author

anp commented Feb 10, 2016

I'm not sure how TravisCI can handle multiple features in the env clause of .travis.yml, due to the double-quotes needed to pass cargo multiple features. I'm going to leave that off and hope that when someone gets a chance to review this PR they can help me figure that out.

@@ -13,6 +13,12 @@

#![cfg_attr(feature = "nightly", feature(drain))]

// optional serde support
#![cfg_attr(feature = "serde_macros", feature(const_fn, custom_derive, plugin))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all of these features be eders instead of serde_macros?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just used the example here (under the section about using both):

https://github.com/serde-rs/serde#using-serde-with-stable-rust-syntex-and-serde_codegen

It doesn't really matter to me, and I'm struggling to think of a scenario where you'd compile with serde support but without those features (unless custom_derive makes it to stable? but I assume other changes would be needed to make it work).

If that's what you'd prefer, I'm happy to make the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I realize what I wrote was ambiguous. What I mean is, I'd expect the line to look like

#![cfg_attr(feature = "eders", feature(const_fn, custom_derive, plugin))]

because that's the Cargo feature the user would enable in order to turn on serde support. (And yes, that will only work on nightly.) The other cfg and cfg_attr attributes should be changed analogously.

@apasel422
Copy link
Contributor

For Travis, it should be possible to add an additional

- rust: nightly
  env: FEATURES="--features eders"

to .travis.yml.

@anp
Copy link
Contributor Author

anp commented Feb 12, 2016

Thanks for taking a look! I'll hopefully be able to try the second nightly test run for Travis later tonight.

@anp
Copy link
Contributor Author

anp commented Feb 12, 2016

Anything you see that I should tweak?

@apasel422
Copy link
Contributor

@dikaiosune Thanks! Could you add a test like

#[cfg(feature = "eders")]
#[test]
fn test_serde() {
    <foo>
}

where <foo> is some really simple code that ensures that VecMap implements the expected traits?

After that, please squash all the commits and this PR should be good to merge.

@anp
Copy link
Contributor Author

anp commented Feb 13, 2016

It looks like the serde build is currently failing due to the combination of some breaking changes to libsyntax and a "^x.x.x" dependency on quasi in serde_codegen (so I can't just pin the bugfix version number in the serde dependency). I'm going to revisit this once the serde folks get the breaking changes sorted out.

@anp
Copy link
Contributor Author

anp commented Feb 15, 2016

OK, test code is in, commits are squashed. I'm not a huge fan of the method I used for the test, but I didn't want to ask to introduce a dev dependency on a crate that would provide Serializer/Deserializer to directly test the trait implementations. Let me know what you think.

@@ -1458,4 +1465,22 @@ mod test {
assert_eq!(a[&2], "two");
assert_eq!(a[&3], "three");
}

// test serde trait implementations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An even simpler approach would be to replace all the code from here onward with:

#[test]
#[cfg(feature = "eders")]
fn test_serde() {
    use serde::{Serialize, Deserialize};

    fn impls_serde_traits<S: Serialize + Deserialize>() {}

    impls_serde_traits::<VecMap<u32>>();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is naive, but I wanted to make sure it actually compiled if it wasn't ever called. Does type checking happen before dead code is pruned?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, dead code still has to type-check.

Adding eders feature for separate TravisCI nightly build.
Adding compilation test for serde traits.
@anp
Copy link
Contributor Author

anp commented Feb 15, 2016

Updated. Thanks very much for your patience.

apasel422 added a commit that referenced this pull request Feb 15, 2016
Adding nightly serde support for VecMap under "eders" feature flag
@apasel422 apasel422 merged commit 240002b into contain-rs:master Feb 15, 2016
@apasel422
Copy link
Contributor

@dikaiosune No problem! Thanks for the PR!

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.

None yet

2 participants