Skip to content

url: add TryFrom<&[u8]> for Url and TryFrom tests #638

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

Closed
wants to merge 1 commit into from

Conversation

acfoltzer
Copy link

Per discussion on Matrix.

I didn't realize when first asking that TryFrom<&str> is already implemented, but hasn't been released. So, I ended up just adding the impl for byte slices, an error variant for invalid UTF-8 inputs, and tests for the impls.

Per [discussion on Matrix](https://matrix.to/#/!wurHQemAfsdBxUYHeU:mozilla.org/$MLu4rFb7uBI54AT4-UwKZSyEMXELJRM7Rqm1dpGZ6kM?via=mozilla.org&via=matrix.org).

I didn't realize when first asking that `TryFrom<&str>` is already implemented, but hasn't been
released. So, I ended up just adding the impl for byte slices, an error variant for invalid UTF-8
inputs, and tests for the impls.
@acfoltzer
Copy link
Author

I believe CI is failing because a credential is not available from my fork.

@SimonSapin
Copy link
Member

I feel this should not be implemented. URL parsing is defined over Unicode input. &[u8] and &str are not interchangeable representations of "the same thing", Unicode and bytes are separate "domains" of data and the choice of strict UTF-8 decoding for converting between them should be explicit to users.

(Unfortunately the Matrix link doesn’t seem to work from here.)

@acfoltzer
Copy link
Author

@SimonSapin I definitely agree with you on these being separate domains of data, but I believe that's reflected by the use of TryFrom (rather than From) and the addition of the InvalidUtf8 error variant.

There wasn't much discussion on Matrix really:
image

@djc
Copy link
Contributor

djc commented Aug 31, 2020

FWIW, like @SimonSapin I think this is a bad fit.

@acfoltzer
Copy link
Author

No worries, again it's just a convenience and not a dealbreaker for our adoption.

@acfoltzer acfoltzer closed this Sep 3, 2020
@jsha jsha mentioned this pull request Oct 19, 2020
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.

3 participants