Implement Hash for Origin (fix #302)

[cGuille]

Hello,

I'm a Rust / Open Source beginner, so just explain if anything is wrong. :-)

I am giving a try at issue #302. I just derived the Hash for the Origin type.

I have zero experience with testing in Rust so I just tried to demonstrate that it works, but I do not know how to write a good test for this.

What should I do now?

---

This change is 

[skade]

@cGuille Wait for one of the maintainers to provide review :). Most maintainers of the URL crate live in the US, so this might take until later.

Ignore the nightly error in the linked travis test suite, rust nightly is currently broken, not this lib.

[emilio]

@SimonSapin is french fwiw, hopefully he can take a look soon-ish.

Can you test also that opaque origins get different hashes? (It's trivial, but worth having that in the test-suite too).

[cGuille]

Hello,

There is actually no hurry at all. :-)

I am OK with adding more tests ; I will just have to figure out how.
I do not like using HashMap to indirectly test the Hash trait ; it would be nice to have someone else's opinion about this.

[SimonSapin]

@skade, unfortunately, most of the maintainers of the url crate are me :) I’m usually in UTC+1~2.

@cGuille You can hash without a hashmap like so:

use std::collections::hash_map::DefaultHasher;
use std::hash::Hash;

fn hash<T: Hash>(value: T) -> u64 {
    let mut hasher = DefaultHasher::new();
    value.hash(&mut hasher);
    hasher.finish()
}

To make the test more interesting, you could use different URLs that have the same origin. Please also test that hashing is consistent with == equality.

Thanks!

[cGuille]

Thank you!

I updated the PR to test hash values directly instead of using HashMap, but I do not know how to test that hashing is consistent with equality. Do you have something in mind?
Should I just check that equalities of the origins in my test are the same as the equalities of their hashes?

[SimonSapin]

Test that things that compare equal with == have the same hash. Test that things that compare unequal have a different hash. The latter is not guaranteed because there could be hashing collisions, but it is unlikely enough that it’s alright to test in a couple fixed-input test cases.

[cGuille]

What do you think about this? Are there enough URL samples?
Edit: I also added tests with opaque origins, as @emilio suggested.

[bors-servo]

☔ The latest upstream changes (presumably #306) made this pull request unmergeable. Please resolve the merge conflicts.

[cGuille]

I solved the conflict with master and rebased the branch upon it.

Tell me if there is anything left to do regarding this PR.

[dtolnay]

Looks good to me. All of the feedback so far has been addressed.

[bors-servo]

☔ The latest upstream changes (presumably #344) made this pull request unmergeable. Please resolve the merge conflicts.

[cGuille]

I rebased upon master to solve this second conflict.

Can this be merged?

[cGuille]

@bors-servo r? @SimonSapin

[brson]

@bors-servo r+

bors-servo probably doesn't listen to me...

[bors-servo]

@brson: 🔑 Insufficient privileges: Not in reviewers

[brson]

Sorry for the delay @cGuille !

[cbrewster]

@bors-servo r=brson

[bors-servo]

📌 Commit 4260325 has been approved by brson

[bors-servo]

⌛ Testing commit 4260325 with merge 2641e36...

[bors-servo]

☀️ Test successful - status-travis
Approved by: brson
Pushing 2641e36 to master...