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

Add support for title #224

Merged
merged 2 commits into from
Jul 24, 2022
Merged

Add support for title #224

merged 2 commits into from
Jul 24, 2022

Conversation

Sytten
Copy link
Contributor

@Sytten Sytten commented Jul 20, 2022

This adds support for title.
It's my first time working with functional macros and this is not a simple use-case so let me know if this can be improved.
The title is useful for enums because currently the generator for rust clients created a struct for each variant and name them like:

  • my_enum_one_of_1
  • my_enum_one_of_2
  • etc

So with the title you can at least control the name of each variant structure. There is an open issue to actually produce an enum but it's not been worked on for a while OpenAPITools/openapi-generator#9497.

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

It is pretty good. There are some of the extra fields that are not supported by far and adding support for them is a good thing though and title is one of them.

However this implementation seems bit overly complicated for ther purpose.

  • The Struct in attr.rs file is for attributes supported on Component level.
  • The UnnamedFieldStruct attr.rs file is for attributes supported on Component level of a unnamed struct components.
  • The NamedField in attr.rs file is for attributes supported on a field level of a Component.

By creating another type for title seems superfluous and does not give the the combined error message to the users when they mistype the attribute for the Component.

Based on test in the code the Struct and UnnamedFieldStruct needs changes. Instead of creating another type for the parsing title one could just add following lines to the types.

#[derive(Default)]
#[cfg_attr(feature = "debug", derive(Debug))]
pub struct Struct {
    example: Option<AnyValue>,
    xml_attr: Option<XmlAttr>,
    title: Option<String> // add title to the existing struct same should be added to the UnnamedFieldStruct
}

// then in parse component attr struct
impl Parse for ComponentAttr<Struct> {
    fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
        const EXPECTED_ATTRIBUTE_MESSAGE: &str =
            "unexpected attribute, expected any of: example, xml, title";
// ......
                "title" => {
                    struct_.title = Some(parse_utils::parse_next_literal_str(input)?);
                }
// ......

// then in to tokens struct
impl ToTokens for Struct {
// ....
        if let Some(ref title) = self.title {
            tokens.extend(quote! {.title(#title)});
        }
// ....
}

Then the same for UnnamedFieldStruct.

@Sytten
Copy link
Contributor Author

Sytten commented Jul 24, 2022

The reason I didnt do that is that you don't want it printed at the same level depending on the use case for enums. I initially did as you wrote, but realized that the title was misplaced for the cases of variant_tokens because the variant is set as a property.

We might want to do more like the deprecated support, but I don't think going the route of title in the existing structs is super.
We could make it work if we put the props as pub so they can be removed depending on the usecase but it felt wrong.

@juhaku
Copy link
Owner

juhaku commented Jul 24, 2022

The reason I didnt do that is that you don't want it printed at the same level depending on the use case for enums. I initially did as you wrote, but realized that the title was misplaced for the cases of variant_tokens because the variant is set as a property.

We might want to do more like the deprecated support, but I don't think going the route of title in the existing structs is super. We could make it work if we put the props as pub so they can be removed depending on the usecase but it felt wrong.

Aah.. Right.. Okay then surely that is okay. I didn't figure out that at first. Yeah it otherwise would indeed endup on the underlying type of a property used as a variant. Which is indeed a wrong place for it in this context. Yeah it would feel wrong to me either. There is probably architectural improvements to be done regarding this in future so that the correct attributes would be contained in one place for the type and not becoming scattered too small pieces. Thanks for clarifying ❤️

@juhaku juhaku merged commit eeb0dd4 into juhaku:master Jul 24, 2022
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