Remove need of pest3_core as a dependency for pest3 users #18
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
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 (
|
|
fixes #7 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
generator/src/types.rs (2)
5-12: LGTM! Consider updating documentation for the new feature.The conditional compilation introduced here aligns well with the PR objectives. It allows for a more flexible use of the library, simplifying dependencies for most users while still providing access to core functionality when needed.
Consider adding documentation comments to explain:
- The purpose of the "meta" feature
- When users might need to enable it
- The implications of using
::pest3vs::pest3_coreThis will help users understand when and how to use this feature.
Line range hint
19-22: Consider adding a note about feature-dependent behavior.While this function hasn't changed directly, its behavior now depends on the "meta" feature due to its use of the
pest()function.Consider adding a comment to this function explaining that its behavior changes based on the "meta" feature. This will help users understand the potential differences in the returned
TokenStream.derive/Cargo.toml (1)
Line range hint
1-24: Consider the following improvements to the Cargo.toml file:
Version constraints: The current exact version constraints (= 0.0.0-prealpha0) for dependencies might be too restrictive for future updates. Consider using more flexible version constraints (e.g., ^0.0.0-prealpha0) to allow for compatible updates.
Rust version: It might be helpful to specify the minimum supported Rust version by adding a
rust-versionfield to the[package]section. This helps users understand the compatibility requirements of your package.Here's an example of how you could implement these suggestions:
[package] # ... existing fields ... rust-version = "1.56" # Replace with your minimum supported Rust version [dependencies] pest3_generator = { path = "../generator", version = "^0.0.0-prealpha0" } pest3_core = { path = "../core", version = "^0.0.0-prealpha0" } [dev-dependencies] pest3 = { path = "../pest", version = "^0.0.0-prealpha0" } pest3_generator = { path = "../generator", version = "^0.0.0-prealpha0", features = [ "meta" ] } # ... other dev-dependencies ...These changes would make the package more flexible for future updates while clearly communicating its requirements.
core/src/typed/traits.rs (2)
Line range hint
172-197: Add documentation for public traits and methodsThe newly added public traits (
PairContainer,EmptyPairContainer,PairTree,TypedParser,NeverFailedTypedNode) and their methods lack RustDoc comments. To enhance code readability and usability for other developers, especially external users of the crate, consider adding documentation comments explaining the purpose and usage of each trait and method.
Line range hint
174-178: Consider simplifying method names for clarityThe method name
for_self_or_for_each_child_pairin thePairContainertrait is quite long and may affect readability. Consider renaming it to something more concise, such asfor_each_pair, to make the API more user-friendly and intuitive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- core/src/typed/mod.rs (1 hunks)
- core/src/typed/traits.rs (1 hunks)
- derive/Cargo.toml (1 hunks)
- generator/Cargo.toml (1 hunks)
- generator/src/typed/generator.rs (1 hunks)
- generator/src/types.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
core/src/typed/mod.rs (1)
12-12: LGTM! Change aligns with PR objectives.The addition of
full_rule_structto the public exports is a positive change that aligns well with the PR's objective of simplifying dependency requirements for Pest3 users. By making this macro publicly accessible, users can now utilize it directly without needing thepest3_coredependency.This change is minimal and doesn't affect the existing structure of the module, maintaining backward compatibility while enhancing the public API.
generator/Cargo.toml (1)
9-11: LGTM! Verify the usage of the new 'meta' feature.The addition of the 'meta' feature aligns with the PR objective of simplifying dependencies for Pest3 users. This flag feature allows for more granular control over the package's functionality.
To ensure the feature is properly utilized, please run the following script:
If no usage is found, consider adding appropriate conditional compilation or documentation to explain how this feature should be used.
✅ Verification successful
Verified!
The 'meta' feature is properly utilized within the codebase as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the 'meta' feature in the codebase. # Test: Search for conditional compilation or usage of the 'meta' feature rg --type rust '#\[cfg\(feature\s*=\s*"meta"\)\]' || echo "No usage of 'meta' feature found in conditional compilation" # Test: Search for other potential uses of the 'meta' feature rg --type rust 'feature\s*=\s*"meta"' || echo "No other usage of 'meta' feature found"Length of output: 252
generator/src/types.rs (1)
Line range hint
1-22: Overall, these changes effectively support the PR objectives.The modifications to the
pest()function, along with the introduction of the "meta" feature, successfully simplify the dependency structure for most users while maintaining flexibility for advanced use cases. This aligns well with the PR's goal of removing the need forpest3_coreas a direct dependency for typicalpest3users.To further improve this change:
- Consider adding comprehensive documentation about the "meta" feature and its implications.
- Ensure that any other parts of the codebase that might be affected by this change (e.g., tests, examples, or documentation) are updated accordingly.
- It might be beneficial to add a migration guide or release notes to help existing users adapt to these changes, especially if they were directly using
pest3_core.derive/Cargo.toml (1)
18-18: LGTM! This change aligns with the PR objectives.The addition of the "meta" feature to
pest3_generatorin the dev-dependencies is a positive change. It enables the new functionality for testing purposes without affecting the main package dependencies, which aligns well with the PR's goal of simplifying dependency requirements for Pest3 users.This modification allows for more flexible testing capabilities while maintaining the existing version constraints, ensuring compatibility.
core/src/typed/traits.rs (1)
170-170: 🛠️ Refactor suggestionSimplify macro export for
full_rule_structThe
full_rule_structmacro is being exported using both#[macro_export]andpub use full_rule_struct;, which is redundant. In modern Rust, it's recommended to usepub useto re-export macros from modules instead of using#[macro_export], which places the macro at the crate root and can lead to namespace pollution. Consider removing the#[macro_export]attribute to avoid redundancy and potential conflicts.Apply this diff to remove
#[macro_export]:-#[macro_export] /// Struct for rules with full capacity. macro_rules! full_rule_struct { // macro body }Likely invalid or redundant comment.
| @@ -251,7 +251,7 @@ fn create_rule<'g>( | |||
| quote! { | |||
| #[allow(unused_imports)] | |||
| use #this::typed::SubRule as _; | |||
| #this::full_rule_struct!(#name, (#(#args),*), #rule, #rule_val, #inner_type, #content_type, ); | |||
| #this::typed::full_rule_struct!(#name, (#(#args),*), #rule, #rule_val, #inner_type, #content_type, ); | |||
There was a problem hiding this comment.
💡 Codebase verification
Incomplete Refactoring Detected
Multiple instances of ::pest3_core::full_rule_struct! remain in the codebase. Please update these to use the typed::full_rule_struct! prefix to ensure the pest3_core dependency is fully removed and the refactoring is consistent across all files.
Affected files include:
pest/tests/predefined_node.rsgenerator/tests/expected_import_inline.rsgenerator/tests/expected_json.rsgenerator/tests/expected_sample.rsgenerator/tests/expected_import_dag.rsgenerator/tests/expected.rs
🔗 Analysis chain
LGTM! This change aligns with the PR objective.
The modification of the macro invocation from #this::full_rule_struct! to #this::typed::full_rule_struct! is correct and aligns with the PR's goal of removing the need for pest3_core as a dependency. This change likely moves the macro definition into a typed module within the current crate, which is a step towards making Pest3 more self-contained.
To ensure this change is consistent across the codebase, let's verify the usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of full_rule_struct! without the typed:: prefix
rg --type rust 'full_rule_struct!' | grep -v 'typed::full_rule_struct!'
Length of output: 10599
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
core/src/typed/traits.rs (4)
170-171: LGTM! Consider adding documentation for thefull_rule_structmacro.The
full_rule_structmacro is well-implemented and provides a useful way to generateTypedNodeimplementations. It correctly handles error tracking and span management.Consider adding documentation for the
full_rule_structmacro to explain its purpose and usage. This will help other developers understand when and how to use this macro.
Line range hint
173-186: LGTM! Consider optimizingvec_children_pairsfor large containers.The
PairContainertrait is well-designed and provides useful methods for handling pairs. The default implementation forfor_self_or_for_each_child_pairis a nice touch.For the
vec_children_pairsmethod, consider adding a default capacity to the vector to reduce allocations for larger containers. For example:fn vec_children_pairs(&self) -> Vec<Pair<R>> { let mut vec = Vec::with_capacity(16); // Or another appropriate initial capacity self.for_each_child_pair(&mut |token| vec.push(token)); vec }
Line range hint
202-220: LGTM! Consider optimizingas_pair_treefor large trees.The
PairTreetrait is well-designed and provides useful methods for working with pair trees. It extendsPairContainerappropriately and includes methods for getting rules and spans.For the
as_pair_treemethod, consider adding a way to reuse existingPairstructs or preallocate thechildrenvector to reduce allocations for larger trees. For example:fn as_pair_tree(&self) -> Pair<R> { let rule = Self::get_rule(); let (start, end) = self.get_span(); let mut children = Vec::with_capacity(16); // Or another appropriate initial capacity self.for_each_child_pair(&mut |token| children.push(token)); Pair { rule, start, end, children, } }
Line range hint
265-315: LGTM! Consider adding more documentation forNeverFailedTypedNode.The
TypedParserandNeverFailedTypedNodetraits are well-designed and provide useful abstractions for parsing. TheTypedParsertrait nicely wraps theTypedNodemethods, providing a clear interface for parsing.For the
NeverFailedTypedNodetrait, consider adding more documentation to explain its use case and when it should be implemented. This will help other developers understand when to use this trait instead of the regularTypedNodetrait. For example:/// Node of concrete syntax tree that never fails to parse. /// /// This trait should be implemented for nodes that are guaranteed to always /// successfully parse, such as optional elements or alternatives where one /// option always matches. It provides a simpler interface compared to `TypedNode` /// as it doesn't need to handle failure cases. pub trait NeverFailedTypedNode<'i, R: RuleType> where Self: Sized + Debug + Clone + PartialEq + Default, { // ... existing methods ... }generator/tests/expected.rs (1)
Inconsistent Namespace in
full_rule_struct!Macro CallsThe verification script identified instances where the
full_rule_struct!macro is used without the::pest3_core::typed::namespace:
full_rule_struct!(A, (), Rule, Rule::Foo, Empty, Box<Empty>,);Please update all active macro calls to use the correct namespace to ensure consistency:
::pest3_core::typed::full_rule_struct!(A, (), Rule, Rule::Foo, Empty, Box<Empty>,);Additionally, review any macro usages in comments to determine if they need updating or can remain as references.
🔗 Analysis chain
Line range hint
78-2141: Consistent Update tofull_rule_struct!Macro NamespaceAll instances of the
full_rule_struct!macro have been updated to use the::pest3_core::typednamespace, reflecting the transition to the typed version of the parsing framework. This change is consistently applied across all rule structures and appears correct.To ensure that all macro calls have been updated appropriately and no instances have been missed, please run the following script:
This script searches for all occurrences of
full_rule_struct!and filters out those that are correctly prefixed with::pest3_core::typed::. Any output would indicate macro calls that have not been updated to the new namespace.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `full_rule_struct!` use the `::pest3_core::typed` namespace. # Expected: No output indicates all macro calls are correctly updated. rg --type rust --no-heading --no-filename 'full_rule_struct!' | \ rg -v '::pest3_core::typed::full_rule_struct!'Length of output: 297
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- core/src/typed/mod.rs (1 hunks)
- core/src/typed/traits.rs (1 hunks)
- derive/Cargo.toml (1 hunks)
- generator/Cargo.toml (1 hunks)
- generator/src/typed/generator.rs (1 hunks)
- generator/src/types.rs (1 hunks)
- generator/tests/expected.rs (27 hunks)
- generator/tests/expected_import_inline.rs (8 hunks)
- generator/tests/expected_json.rs (25 hunks)
- generator/tests/expected_sample.rs (75 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/typed/mod.rs
- derive/Cargo.toml
- generator/Cargo.toml
- generator/src/typed/generator.rs
- generator/src/types.rs
🧰 Additional context used
🔇 Additional comments (10)
core/src/typed/traits.rs (1)
Line range hint
196-200: LGTM!EmptyPairContainertrait is well-designed.The
EmptyPairContainertrait and its implementation forPairContainerare simple, efficient, and serve their purpose well. This design allows for easy handling of empty containers in the parsing system.generator/tests/expected_import_inline.rs (8)
Line range hint
71-82: Confirm updated macro path forr#xruleThe
full_rule_struct!macro invocation for ther#xrule has been correctly updated to use::pest3_core::typed::full_rule_struct!. This change aligns with the new public accessibility of the macro underpest3_core::typed, allowing users to access it directly without needing thepest3_coredependency.
Line range hint
83-100: Verify correctness ofcvt_into()usage in macroIn the macro invocation at line 83, the method
super::Rule::r#x.cvt_into()is used when specifying the rule. Please verify that thecvt_into()method is properly implemented and that it correctly convertssuper::Rule::r#xinto the expected super rule type. This ensures seamless integration and correct rule identification during parsing.
Line range hint
177-206: Update macro path forr#wruleThe
full_rule_struct!macro invocation for ther#wrule has been updated to use::pest3_core::typed::full_rule_struct!. This is consistent with the changes made to make the macro publicly accessible and reduces unnecessary dependencies.
Line range hint
237-266: Update macro invocation and helper function inr#x0The macro invocation for
r#x0has been correctly updated to use the new path::pest3_core::typed::full_rule_struct!. Additionally, the helper functionr#xis implemented to return a reference tominimal::rules::r#x, providing convenient access to the matched content.
Line range hint
316-344: Check helper functions inr#x1The helper functions
r#wandr#xinr#x1correctly provide access to their respective matched rules. Ensure that these functions accurately represent the grammar(minimal::x - w)and that the sequence structure is implemented properly.
Line range hint
407-436: Confirm tuple handling inr#x2helper functionThe helper function
r#xinr#x2returns a tuple of references tominimal::rules::r#x. This implementation should correctly represent the grammar(minimal::x - minimal::x). Please confirm that tuple handling is as intended and that both elements are appropriately accessed.
Line range hint
489-518: Review implementation ofr#x3with string matchIn
r#x3, the helper functionr#xprovides access tominimal::rules::r#x, corresponding to the grammar(minimal::x - "y"). Ensure that the sequence and exclusion with the string"y"are correctly handled and that the implementation aligns with the intended parsing behavior.
Line range hint
569-598: Validate choice pattern inr#x4The
r#x4rule uses aChoice2generic to represent the grammar(minimal::x | "z"). The helper functionr#xreturns anOption<&minimal::rules::r#x>, which isSomeifminimal::xwas matched andNoneotherwise. Verify that this choice pattern correctly handles both possibilities and that the parsing logic functions as expected.generator/tests/expected_sample.rs (1)
118-118: Ensure all macro invocations are correctly updated to the new namespaceThe code updates the
full_rule_struct!macro invocations to use the new namespace::pest3_core::typed::full_rule_struct!. Please verify that all occurrences offull_rule_struct!in the codebase have been updated to avoid any inconsistencies or build issues.Run the following script to check for any instances of
full_rule_struct!not using the new namespace:Also applies to: 171-171, 224-224, 284-284, 344-344, 397-397, 452-452, 533-533, 624-624, 715-715, 789-789, 877-877, 949-949, 1021-1021, 1093-1093, 1156-1156, 1219-1219, 1288-1288, 1358-1358, 1426-1426, 1495-1495, 1565-1565, 1633-1633, 1707-1707, 1788-1788, 1859-1859, 1935-1935, 2016-2016, 2087-2087, 2158-2158, 2229-2229, 2300-2300, 2371-2371, 2442-2442, 2513-2513, 2612-2612, 2704-2704, 2783-2783, 2855-2855, 2919-2919, 2965-2965, 3025-3025, 3085-3085, 3145-3145, 3205-3205, 3321-3321, 3456-3456, 3556-3556, 3644-3644, 3747-3747, 3857-3857, 3989-3989, 4118-4118, 4228-4228, 4336-4336, 4525-4525, 4657-4657, 4739-4739, 4821-4821, 4903-4903, 4985-4985, 5067-5067, 5149-5149, 5231-5231, 5313-5313, 5393-5393, 5470-5470, 5566-5566, 5653-5653, 5724-5724, 5798-5798, 5872-5872, 5943-5943, 5998-5998, 6056-6056
| @@ -118,7 +118,7 @@ pub mod rules { | |||
| } | |||
| #[allow(unused_imports)] | |||
| use pest3_core::typed::SubRule as _; | |||
| ::pest3_core::full_rule_struct!( | |||
| ::pest3_core::typed::full_rule_struct!( | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Repeated full_rule_struct! Macro Calls to Reduce Duplication
The use of full_rule_struct! macro with similar parameters is repeated across multiple rules (e.g., lines 121, 149, 264, etc.). This repetition can make the code harder to maintain. Consider creating a helper macro or function to abstract common patterns and reduce code duplication.
Now a user has to put just one line in the dependencies:
In order to do that I have:
full_rule_structmacro public (even to the user)metafeature to the generator crate, so that I can activate it in the generator's tests.Summary by CodeRabbit
New Features
full_rule_structmacro for enhanced parsing capabilities.PairContainer,EmptyPairContainer,PairTree,TypedParser, andNeverFailedTypedNodefor improved functionality in handling typed nodes and pairs.metafeature in thepest3_generatorpackage.Bug Fixes
Documentation