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 "" as the fallback from desc locales #29

Closed
wants to merge 2 commits into from
Closed

Use "" as the fallback from desc locales #29

wants to merge 2 commits into from

Conversation

circuitsacul
Copy link
Contributor

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.

@circuitsacul circuitsacul mentioned this pull request Jul 5, 2023
40 tasks
@baptiste0928
Copy link
Owner

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?

Copy link
Owner

@baptiste0928 baptiste0928 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -42,8 +42,11 @@ pub fn impl_create_command(input: DeriveInput, fields: Option<FieldsNamed>) -> R
};
let name_localizations = localization_field(&attributes.name_localizations);
let description = match &attributes.desc {
Some(desc) => desc.clone(),
None => parse_doc(&input.attrs, span)?,
Some(desc) => quote! { ::std::option::Option::Some((#desc).to_string()) },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trait methods should be explicit in macro expansion, since there can be multiple traits implementing a method with the same name in user code. It's better to use From for consistency with existing code.

Suggested change
Some(desc) => quote! { ::std::option::Option::Some((#desc).to_string()) },
Some(desc) => quote! { ::std::option::Option::Some(::std::convert::From::from(#desc)) },

Some(desc) => desc.clone(),
None => parse_doc(&input.attrs, span)?,
Some(desc) => quote! { ::std::option::Option::Some((#desc).to_string()) },
None => match parse_doc(&input.attrs, span).ok() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change the parse_doc signature to Result<Option<String>, _> instead, errors should not be ignored silently.

@circuitsacul
Copy link
Contributor Author

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.

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.

2 participants