Enable outer and inner attributes on enum with all unit variants.#35
Enable outer and inner attributes on enum with all unit variants.#35tomtau merged 1 commit intopest-parser:masterfrom
Conversation
…umed in unit variants of enum
WalkthroughThe changes introduce a more sophisticated attribute handling system for the Changes
Sequence DiagramsequenceDiagram
participant Macro as FromPest Derive Macro
participant Attributes as Attribute Handlers
participant FieldConverter as Field Converter
Macro->>Attributes: Parse Field Attributes
Attributes-->>Macro: Attribute Structures
Macro->>FieldConverter: Convert Fields
alt Struct Fields
FieldConverter->>Macro: struct_convert
else Enum Variants
FieldConverter->>Macro: enum_convert
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
derive/src/from_pest/field.rs (1)
Line range hint
141-170: Reduce code duplication betweenenum_convertandstruct_convertBoth
enum_convertandstruct_convertfunctions share similar logic when handlingFields::NamedandFields::Unnamed. Duplicated code can lead to maintenance issues.Consider extracting the common logic into a helper function that can be utilized by both conversion functions. This will improve code reuse and maintainability.
derive/src/from_pest/mod.rs (1)
Line range hint
180-193: Ensure correct variant name usage inderive_for_enumThe
variant_namevariable is introduced to represent&variant.ident. However, in thequote!macro,stringify!(#variant_name)may not correctly interpolate the variant name as intended.Adjust the usage to ensure that the variant name is correctly expanded:
let variant_name = &variant.ident; let construct_variant = field::enum_convert(&parse_quote!(#name::#variant_name), &variant)?; let extraneous = crate::trace(quote! { - "when converting {}, found extraneous {:?}", stringify!(#name), stringify!(#variant_name) + concat!("when converting ", stringify!(#name), "::", stringify!(#variant_name), ", found extraneous") });
🧹 Nitpick comments (3)
derive/src/from_pest/field.rs (2)
Line range hint
35-49: Simplify theConversionStrategy::from_attrsmatching logicThe current implementation of
ConversionStrategy::from_attrsuses a match statement that could be simplified for better readability and maintainability. Specifically, handling the case where multiple attributes are provided can be made clearer.Apply this diff to refactor the matching logic:
fn from_attrs(attrs: Vec<FieldAttribute>) -> Result<Self> { let mut attrs = attrs.into_iter(); - Ok(match (attrs.next(), attrs.next()) { - (Some(_), Some(attr)) => Err(Error::new( - attr.span(), - "only a single field attribute allowed", - ))?, - (None, None) => ConversionStrategy::FromPest, - (Some(FieldAttribute::Outer(attr)), None) => ConversionStrategy::Outer( - attr.span(), - attr.with.into_iter().map(|attr| attr.path).collect(), - ), - (Some(FieldAttribute::Inner(attr)), None) => ConversionStrategy::Inner( - attr.span(), - attr.with.into_iter().map(|attr| attr.path).collect(), - attr.rule.map(|attr| { - let path = attr.path; - let variant = attr.variant; - parse_quote!(#path::#variant) - }), - ), - _ => unreachable!(), - }) + match (attrs.next(), attrs.next()) { + (Some(attr), Some(_)) => Err(Error::new( + attr.span(), + "only a single field attribute allowed", + )), + (None, None) => Ok(ConversionStrategy::FromPest), + (Some(FieldAttribute::Outer(attr)), None) => Ok(ConversionStrategy::Outer( + attr.span(), + attr.with.into_iter().map(|attr| attr.path).collect(), + )), + (Some(FieldAttribute::Inner(attr)), None) => Ok(ConversionStrategy::Inner( + attr.span(), + attr.with.into_iter().map(|attr| attr.path).collect(), + attr.rule.map(|attr| { + let path = attr.path; + let variant = attr.variant; + parse_quote!(#path::#variant) + }), + )), + _ => unreachable!(), + } }
133-137: Handle unit variants without using a placeholder memberIn the
Fields::Unitcase withinenum_convert, the code applies the conversion strategy to aMember::Unnamed(Index::from(0)), which doesn't conceptually represent anything for unit variants. This could be misleading.Consider adjusting the implementation to apply the conversion strategy without using a placeholder member:
Fields::Unit => { let attrs = FieldAttribute::from_attributes(variant.attrs.clone())?; - let real_name = - ConversionStrategy::from_attrs(attrs)?.apply(Member::Unnamed(Index::from(0))); - quote!(#real_name) + let strategy = ConversionStrategy::from_attrs(attrs)?; + match strategy { + ConversionStrategy::FromPest => quote!(#name), + _ => strategy.apply(Member::Named(variant.ident.clone())), + } }derive/src/attributes.rs (1)
Line range hint
35-49: Add documentation for the newFieldAttributeenum variantsThe
FieldAttributeenum has been introduced withOuterandInnervariants, but lacks comprehensive documentation explaining their usage and significance.Consider adding Rust doc comments to describe the purpose and usage of
FieldAttribute,OuterAttribute, andInnerAttribute./// `#[pest_ast(..)]` for fields in `#[derive(FromPest)]` #[derive(Debug)] +/// Represents field-level attributes for the `FromPest` derive macro. +/// +/// Variants: +/// - `Outer`: Denotes that the field should be parsed from an outer rule. +/// - `Inner`: Indicates that the field should be parsed from an inner rule, potentially with a specific rule and additional attributes. pub(crate) enum FieldAttribute { /// `outer(with(path::to),*)` Outer(OuterAttribute), /// `inner(rule(path::to), with(path::to),*)` Inner(InnerAttribute), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
derive/src/attributes.rs(3 hunks)derive/src/from_pest/field.rs(3 hunks)derive/src/from_pest/mod.rs(2 hunks)
🔇 Additional comments (3)
derive/src/from_pest/mod.rs (1)
Line range hint
136-154: Verify the handling of generated lifetimesIn the
derivefunction, when synthesizing a new lifetime'unique_lifetime_name, it is essential to ensure that this does not conflict with existing lifetimes or cause unintended behavior.Please confirm that introducing the
'unique_lifetime_namelifetime does not introduce any conflicts in the generated code, especially when dealing with complex generics or existing lifetimes.derive/src/attributes.rs (2)
51-58: Ensure proper parsing ofOuterAttributeandInnerAttributeThe parsing implementations for
OuterAttributeandInnerAttributeshould handle edge cases, such as empty lists or unexpected tokens.Please review the
parseimplementations for these structs to ensure they correctly handle all possible inputs and provide meaningful error messages.
67-69: ImplementParseandToTokensforWithAttributeWhile
WithAttributeis defined, ensure that itsParseandToTokenstraits are properly implemented to integrate seamlessly with the syn and quote crates.The implementations for
ParseandToTokensforWithAttributeare correctly provided, enabling it to be used in attribute parsing.
tomtau
left a comment
tomtau
left a comment
There was a problem hiding this comment.
@TonyWu20 thanks for your contribution. pest-ast is passively maintained and not that widely used (at least among OSS projects), so it's a bit hard to tell whether this change breaks any existing behaviour off the top of my head, but it looks it should hopefully be all right.
One way to improve it is perhaps to abstract the duplicate common code between convert_struct and convert_enum, but ok to merge it as it is
2 participants
This is a quick and dirty hack. I was attempting to get the pest-at derive works on my enums with all variants are unit type. The enum is like this:
And the formatted output and input to be parsed are in the same format like:
MyEnum : A, and it is case-insensitive.So I wrote my
.pestgrammar as follows:To achieve the conversion from
&strtoMyEnum, I use my own proc-macro derive crate to generate code like this:and then I added attributes to each variants first.
The original
pest-astcodebase does not apply the conversion on unit variant, as I found inderive/src/from_pest/field.rs. The reason is theconvertfunction accepts(name: &Path, fields: Fields)andFieldsis not available for unit variant in enum, provided by the dependencysyncrate. However, one can still access to the attributes macros on unit variants byvariant.attrs.Therefore, I edited the
convertfunction infield.rsto accept parametervariant: &Variantfor enum case, and leave the original version to be used for struct case. This makes me pass all the existing tests inastbycargo test.Still, I don't have the time and knowledge to fully test this with more potential error-prone cases. In my current usage, it seems my mod only need and only can recognise one
#[pest_ast]outer or inner attribute. For my intended purpose this is workable enough, since all variants share the same rule and same method to convert to the enum. However, it could be not powerful enough to handle cases with more customisation needed. Finally, as I have zero experience with the development ofpestandpest-ast, I believe this kind of mod is of low quality. I was also surprised to see why it seems nobody cares about usingpest-ast, this incredible crate, on enum with unit variants. Hopefully, experienced developers ofpestcan see my efforts and discuss if we can make a better version for this demand. Thank you.Summary by CodeRabbit
Refactor
New Features