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

TrieMap got removed from libcollections #44

Closed
zsiciarz opened this issue Dec 21, 2014 · 8 comments
Closed

TrieMap got removed from libcollections #44

zsiciarz opened this issue Dec 21, 2014 · 8 comments

Comments

@zsiciarz
Copy link

rust-lang/rust/pull/19955 killed TrieMap and a few other collection types. quickcheck provides an Arbitrary impl for TrieMap, but not for any other of the removed collections. What would make more sense - following stdlib and killing the TrieMap impl altogether, or using the collect crate as dependency?

@BurntSushi
Copy link
Owner

That's a great question. I don't know what the right answer is. On the one hand, there is value (IMO) in keeping the dependencies limited to only the standard library. On the other hand, collect seems to be gaining traction and has other data structures that might be worth writing Arbitrary impls for.

I think we can have our cake and eat it too. What if I created a new crate inside this repository that contained impls for Arbitrary from third party libraries? (Starting with collect-rs.)

@BurntSushi
Copy link
Owner

@zsiciarz P.S. Love your 24 days of Rust blog ports. :D

@japaric
Copy link
Contributor

japaric commented Dec 21, 2014

What if I created a new crate inside this repository that contained impls for Arbitrary from third party libraries?

Sadly, you can't do that. You need to implement Arbitrary for external structs in the quickcheck crate because that's where Arbitrary is defined. Otherwise you'll end with:

extern crate collect;
extern crate quickcheck;

use quickcheck::Arbitrary;
use collect::TrieMap;

impl<A: Arbitrary> Arbitrary for TrieMap<A> {}
//~^ error: cannot provide an extension implementation where both trait and type are not defined in this crate

@BurntSushi
Copy link
Owner

... duh. I must have been out of sorts this morning.

OK. I'm not a big fan of it, but I think I'll just keep the impl and the dependency inside QuickCheck proper for now. If we run into problems or if it becomes a maintenance burden, we can re-evaluate.

(If there's anything I've missed or if someone else wants to weigh in, please do!)

@japaric
Copy link
Contributor

japaric commented Dec 23, 2014

@BurntSushi Cargo has this "features" ... feature (which I haven't tried yet) that let's you specify optional dependencies. Do you think it would be a good idea to add a "collect" feature that makes both the dependency on collect-rs and the impl Arbitratry for TrieMap optional? That way the default quickcheck build would be dependency-free.

@BurntSushi
Copy link
Owner

@japaric That sounds like another good idea. How about we retain the status quo until there is a problem? I don't mind collecting impls, but if the deps break a lot, it could be a maintenance burden.

An argument in favor of these impls is that they aren't exactly trivial to write. :-/

@japaric
Copy link
Contributor

japaric commented Dec 24, 2014

@BurntSushi Personally, I'd like to make the default build of quickcheck dependency-free because that'd follow Rust's "pay for what you use" philosophy. If you need TrieMaps then you can opt-in via the collect feature and you pay with one more cargo dependency (that may break daily nightly :-)).

If interested I can send a PR, it's just an 8 line commit.

@BurntSushi
Copy link
Owner

Yeah, that does sound good, particularly in the midst of a broken dependency. I can't test any of my code!

Send the PR on over. Thanks. :D

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

No branches or pull requests

3 participants