baptiste0928 / twilight-interactions

Macros and utilities to work with Discord Interactions using twilight.
ISC License
37 stars 7 forks source link

Use "" as the fallback from desc locales #29

Closed circuitsacul closed 1 year ago

circuitsacul commented 1 year ago

I decided to try my hand at this, since I hadn't worked with proc macros before.

This implementation uses the "" locale from the function as a fallback if there's no doc comment or desc attribute. This also means that ::create_command returns a result. The errors probably need some work.

I also added another test module for option descriptions, since that doesn't seem to be anywhere already.

I'm not sure if there's any other places where locale functions are used that could actually use this - I know that names cannot, since those are stored statically on the structs on expanding, at which point we don't have access to the HashMap of locales. Currently this handles command descriptions and option descriptions. Do choices have descriptions that would use this too?

I know you wanted to decide what implementation to use, which is fine. I wanted to do this anyways, even though there's a decent chance this won't be used. Since I actually succeeded I figured I'd open a draft pr so you could at least see it.

baptiste0928 commented 1 year ago

Thanks for the PR! I accidentally turned off GitHub Actions checks for pull requests in a previous commit (fixed with #30). Can you merge main into your branch to have the checks run?

circuitsacul commented 1 year ago

Thanks for your work on this!

Regarding the feature design, I don't really like the fact that we lose compile-time validation. Maybe we can require the function to return a struct that explicitly defines the fallback instead:

fn locale_option() -> DescLocalizations {
    let locales = [("en", "en description")];

    // Using a constructor here to allow users to use different types:
    // locales => impl IntoIterator<Item = (ToString, ToString)>
    // fallback => impl ToString
    DescLocalizations::new(locales, "fallback description")
}

We can introduce a similar NameLocalizations type for consistency. What do you think of this alternative design?

I like it, the only concern I have is that this breaks all existing locale functions. This was already going to be a breaking change, so ig it's not that big a deal.