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

Use AsRef as bounds in negotiation #15

Merged
merged 1 commit into from
Sep 15, 2019

Conversation

cmyr
Copy link
Contributor

@cmyr cmyr commented Sep 14, 2019

Looking through #14, I was left wondering if something like this wouldn't be a simpler way to get where you want to be?

(This is just intended as an illustration, not as a real patch.)

@zbraniecki
Copy link
Collaborator

Hmm, I like the approach!

@cmyr
Copy link
Contributor Author

cmyr commented Sep 14, 2019

And I think this will work for Locale with just,

impl AsRef<LanguageIdentifier> for Locale {
    fn as_ref(&self) -> &LanguageIdentifier {
        &self.langid
    }
}

@zbraniecki
Copy link
Collaborator

Already there! https://github.com/zbraniecki/unic-locale/blob/master/unic-locale-impl/src/lib.rs#L136

I think this model works quite well for negotiation and other cases where you get something, operate on it as LanguageIdentifier, and then return filtered results etc. (negotiation use case).

But for cases like PluralRules and DateTimeFormat or Localization where your API has to accept a langid/locale or a list of locales/langids, and keep it, would AsRef work?

struct FluentBundle {
    langid: LanguageIdentifier
}

impl FluentBundle {
    pub fn new(langid: AsRef<LanguageIdentifier>) -> Self {
        LanguageIdentifier {
            langid: langid.as_ref().to_owned()
        }
    }
}

struct Localization {
    langids: Vec<LanguageIdentifier>
}

impl Localization {
    pub fn new(langids: &[AsRef<LanguageIdentifier>]) -> Self {
        LanguageIdentifier {
            langids: langid.iter().map(|langid| langid.as_ref().to_owned()).collect()
        }
    }
}

Is that how it would work? Then:

  1. If someone uses Locale they can pass it and we'll turn it into LanguageIdentifier
  2. If we internally switch to Locale we'll use the impl AsRef<Locale> for LanguageIdentfier which will allocate a new Locale out of a LanguageIdentifier?
  3. But if you pass a LanguageIdentifier, we'll allocate a new one via as_ref().to_owned(), right?

@cmyr
Copy link
Contributor Author

cmyr commented Sep 14, 2019

I think the idiomatic API for this would be to use Into<LanguageIdentifier> in the signatures, and then implement From<Locale> for LanguageIdentifier.

Something to think about when designing APIs in Rust is to make allocations explicit. This is informally specified in a bunch of naming conventions: as_ should be used for reference-to-reference conversions, to_ should be used for reference-to-owned conversions (which may be expensive), and into_ is used for owned-to-owned conversions, which... I'm not sure.

Anyway what I mean to say here: AsRef in a function signature suggests that we only want to borrow something. If we want an owned copy we can use Into<T>, which better communicates our intent.

Similarly in general I don't think API should take a slice as an argument if it's going to immediately allocate a vec and copy all the items.

In this particular situation, though, &[impl Into<LanguageIdentifier>] is understandable, because there's no other good way to express what you'd like. The alternative would be to ask for an explicit Vec<LanguageIdentifier>, and to make the user do the allocation themselves before calling the method; or you could make Localization generic over, say, T: AsRef<LanguageIdentifier> but that really doesn't feel worth it?

@cmyr
Copy link
Contributor Author

cmyr commented Sep 14, 2019

Another little thing I noticed looking at locale-impl; you don't need to implement Into<T> for U when both T and U are defined in your crate; From<U> for T gives you an impl automatically. See the From docs for a better explanation.

edit: it's possible I misread your code and this is an external type, in which case carry on 😎

In case you end up using this approach I've fixed the signatures so you can now pass in a slice of owned values and everything just works.

@zbraniecki
Copy link
Collaborator

that looks great! Thank you!

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.

None yet

2 participants