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

IDNA API does not feel optimal when reusing Errors type #359

Open
SamWhited opened this issue Jun 12, 2017 · 3 comments
Open

IDNA API does not feel optimal when reusing Errors type #359

SamWhited opened this issue Jun 12, 2017 · 3 comments

Comments

@SamWhited
Copy link
Contributor

SamWhited commented Jun 12, 2017

Please pardon this slightly generic report; this isn't a specific issue per say, just an API concern (that may already have en easy fix and be my fault, or may be an API problem that would be harder to resolve):

Currently the uts46::Errors vec does not implement Eq or PartialEq, which makes sense since there are ordering concerns (is [Error1, Error2] the same as [Error2, Error1]?) which could potentially make the comparison more resource intense than expected.

However, this feels a bit poor when you're re-using errors in another API. For example, if I have a "dial" method that makes a TCP connection, I may want it to take an IP address or a domain. If it's a domain, it might perform the IDNA to_unicode operation and if it's an IP it might want to validate the format. In my package I would have an error enum like:

#[derive(Debug, Eq, PartialEq)]
pub enum Error {
    Addr(net::AddrParseError),
    IDNA(idna::uts46::Errors),
}

but now I can't define PartialEq for my errors struct (eg. if I want to do assert_eq! in tests). I could of course replace Errors with Error, but then I'd be losing information that users probably expect (since the IDNA package itself exposes all errors that occur during processing).

I suspect there's a way to make the Errors type nicer to reuse without implementing Eq/PartialEq, or I may just be missing something obvious that works today (in which case maybe better documentation is in order? Or maybe it's just me?). Thoughts?

@SamWhited SamWhited changed the title API does not feel optimal when reusing Errors type IDNA API does not feel optimal when reusing Errors type Jun 12, 2017
@behnam
Copy link
Contributor

behnam commented Jul 7, 2017

You're right, @SamWhited.

For now, you may want to drop embedding for IDNA errors and look at the Errors list as a black box.

But, since this is good example of the needs of the API, can you explain what would your application expect from the idna API regarding errors? How much detail are you interested in?

@SamWhited
Copy link
Contributor Author

SamWhited commented Jul 7, 2017

For now, you may want to drop embedding for IDNA errors and look at the Errors list as a black box.

Indeed; for now I am not exposing the IDNA error to my users (just an enum variant called IDNA or something), however, since the information does exist in the IDNA library, I suspect people will ask for it later.

But, since this is good example of the needs of the API, can you explain what would your application expect from the idna API regarding errors? How much detail are you interested in?

I think the current level of detail is fine, but I personally don't need anything beyond just "an error occured". Knowing exactly what happened (or how many bad things happened) is just a "nice-to-have" in my book. However, if it exists in the API as it does now, I would expect to be able to reuse it and expose it to my users in my own error enumeration.

My personal, possibly naive, expectation would be that if it's an error then processing needs to stop, because we may end up with invalid output or just waste time on a relatively expensive operation (processing the rest of the string). We can always skip the bad bytes and process the rest of the string manually if we need to get multiple errors and the API stops processing and returns an error each time, but if we process the whole string and return multiple errors, we can't get back that time if we only care about the first error and then would throw the string away as invalid anyways. Then again, this isn't as "expensive" as most of the Unicode operations you end up using it alongside, so maybe I'm overthinking it; just thinking off the top of my head, might all be baloney.

@SimonSapin
Copy link
Member

idna::Errors is intentionally opaque for now. A Vec was a quick way to get something working but I don’t want to stabilize that API.

expectation would be that if it's an error then processing needs to stop

Yes, this is the kind of thing we’d still need to figure out.

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