-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Basic URL canonicalization. Fixes #84 #88
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
Conversation
rust-lang#84. No tests because there's no preexisting network tests.
This may not actually fix #84. There is another |
Think I'll throw in a few unit tests at least. |
I've also stripped trailing |
fn test_canonicalize_idents_by_stripping_dot_git() { | ||
let ident = ident(&Remote(url("https://github.com/PistonDevelopers/piston.git"))); | ||
assert_eq!(ident.as_slice(), "piston-1ad60373965e5b42"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case we change the hashing algorithm in the future, could this test just test equality of all the urls when hashed? (don't actually look at the specific hash).
Could you also add a test for case insensitivity? The one above just calculates one hash but it's not clear from the test that the hash was generated from a lowercase url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will, but this is following the existing pattern.
r=me with a minor comment |
Basic URL canonicalization. Fixes #84
Basic URL canonicalization. Fixes rust-lang#84
The rustfix tool detects it, so let's inform the user what just happened! Closes rust-lang#88
...#84.
No tests because there's no preexisting network tests.