Rename built-in rules to be all lowercase #21
Conversation
Fixes #9 --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/pest-parser/pest3/issues/9?shareId=XXXX-XXXX-XXXX-XXXX).
WalkthroughThe pull request includes a significant refactoring of type aliases and grammar definitions across multiple files, primarily focusing on renaming public type aliases and tokens from uppercase to lowercase for consistency. This includes ASCII-related constructs and built-in rules in the pest parser. Documentation comments have been removed, and some files have minor formatting adjustments, such as ensuring newline characters at the end. Overall, the changes aim to standardize naming conventions without altering existing functionality. Changes
Assessment against linked issues
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 (
|
|
I was playing with Copilot Workspace, it seems it doesn't work that well 🤣 |
![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 (9)
derive/tests/calc.pest (1)
17-17: Minor: Improved file formattingAlthough not visible in the diff, the addition of a newline character at the end of the file is a good practice for consistent file formatting. This minor change improves the overall code quality without affecting the grammar's functionality.
derive/tests/json.pest (1)
Inconsistent comment styles found in
derive/tests/json.pestMultiple comment styles (
#!and//!) are used within the same file, which may lead to documentation inconsistencies or processing issues.
derive/tests/json.pest: 1 line(s) with#!, 5 line(s) with//!🔗 Analysis chain
Line range hint
1-7: Verify comment style consistencyThe comment style for the first line has been changed from
//!to#!. While this change aligns with the pest grammar syntax, it's important to ensure that this doesn't affect documentation generation or violate any project-wide documentation standards.Please confirm that:
- This change is intentional and aligns with the project's documentation practices.
- It doesn't negatively impact any automated documentation generation processes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in comment styles across pest files # Test: Count occurrences of different comment styles echo "Comment style usage in .pest files:" rg -c '^#!' --type pest rg -c '^//!' --type pestLength of output: 191
Script:
#!/bin/bash # Description: Check for consistency in comment styles across .pest files # Test: Count occurrences of different comment styles in .pest files echo "Comment style usage in .pest files:" rg -c '^#!' -g "*.pest" rg -c '^//!' -g "*.pest"Length of output: 274
core/src/typed/template/mod.rs (6)
6-10: LGTM: Consistent renaming, but consider removing redundant attributesThe renaming of
ASCII_NONZERO_DIGITtoascii_nonzero_digitandASCII_BIN_DIGITtoascii_bin_digitaligns with the PR objectives. However, the individual#[allow(non_camel_case_types)]attributes are now redundant due to the module-level attribute.Consider removing the redundant
#[allow(non_camel_case_types)]attributes for each type alias, as they're covered by the module-level attribute on line 1.
14-19: LGTM: Consistent renaming and definition updateThe renaming of
ASCII_OCT_DIGITtoascii_oct_digitandASCII_HEX_DIGITtoascii_hex_digitis consistent with the PR objectives. The definition ofascii_hex_digithas been correctly updated to use the renamedascii_digit, maintaining internal consistency.As mentioned earlier, consider removing the redundant
#[allow(non_camel_case_types)]attributes for each type alias.
23-27: LGTM: Consistent renaming continuesThe renaming of
ASCII_ALPHA_LOWERtoascii_alpha_lowerandASCII_ALPHA_UPPERtoascii_alpha_uppermaintains consistency with the PR objectives.As previously noted, the
#[allow(non_camel_case_types)]attributes for each type alias are redundant and can be removed.
31-35: LGTM: Consistent renaming and definition updatesThe renaming of
ASCII_ALPHAtoascii_alphaandASCII_ALPHANUMERICtoascii_alphanumericcontinues to align with the PR objectives. The definitions have been correctly updated to use the renamed type aliases, maintaining internal consistency.The redundant
#[allow(non_camel_case_types)]attributes can be removed, as mentioned in previous comments.
39-39: LGTM: Final consistent renamingThe renaming of
ASCIItoasciicompletes the consistent application of lowercase naming for built-in rules, aligning with the PR objectives.As with previous instances, the
#[allow(non_camel_case_types)]attribute here is redundant and can be removed.
1-39: Overall: Consistent implementation of PR objectives with minor improvements neededThe changes in this file successfully implement the PR objective of renaming built-in rules to lowercase, improving naming consistency across the codebase without altering functionality. All ASCII-related type aliases have been consistently renamed, and their usages in definitions have been updated accordingly.
Suggestions for improvement:
- Remove the redundant individual
#[allow(non_camel_case_types)]attributes for each type alias, as they're covered by the module-level attribute on line 1.- Consider reinstating or updating the removed documentation comments to maintain code readability and documentation quality.
Would you like assistance in implementing these suggestions?
derive/tests/grammar.pest (1)
Line range hint
70-83: Consider cleaning up or adding a TODO for the commented-out sectionThe commented-out section at the end of the file mentions an "Invalid segment of grammar below (repeated rule)". Consider either removing this section if it's no longer needed or adding a TODO comment if it's meant to be addressed in the future. This will help maintain the cleanliness and clarity of the grammar file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- core/src/typed/template/mod.rs (1 hunks)
- derive/tests/calc.pest (2 hunks)
- derive/tests/grammar.pest (2 hunks)
- derive/tests/json.pest (2 hunks)
- generator/src/typed/module.rs (0 hunks)
- generator/src/typed/output.rs (2 hunks)
- generator/tests/dag/a.pest (1 hunks)
- generator/tests/dag/b.pest (1 hunks)
- generator/tests/dag/c.pest (1 hunks)
- generator/tests/dag/d.pest (1 hunks)
- generator/tests/dag/e.pest (1 hunks)
- generator/tests/dag/f.pest (1 hunks)
- generator/tests/expected_import_inline.rs (1 hunks)
💤 Files with no reviewable changes (1)
- generator/src/typed/module.rs
✅ Files skipped from review due to trivial changes (6)
- generator/tests/dag/a.pest
- generator/tests/dag/b.pest
- generator/tests/dag/c.pest
- generator/tests/dag/d.pest
- generator/tests/dag/e.pest
- generator/tests/dag/f.pest
🧰 Additional context used
🔇 Additional comments (13)
derive/tests/calc.pest (2)
5-5: LGTM: Consistent renaming of built-in rulesThe change from
pest::SOIandpest::EOItopest::soiandpest::eoialigns perfectly with the PR objective of renaming built-in rules to be all lowercase. This enhances consistency across the codebase without altering the grammar's functionality.
Line range hint
1-17: Overall: Consistent implementation of naming convention changesThe changes in this file successfully implement the PR objective of renaming built-in rules to lowercase. This contributes to a more consistent naming convention across the project without altering the grammar's functionality. The minor formatting improvement further enhances code quality. These changes align well with similar modifications made in other files, demonstrating a comprehensive approach to improving code consistency and readability throughout the codebase.
core/src/typed/template/mod.rs (1)
1-2: LGTM: Changes align with PR objectivesThe addition of the module-level
#![allow(non_camel_case_types)]attribute and the renaming ofASCII_DIGITtoascii_digitare consistent with the PR's goal of standardizing built-in rules to lowercase. This change improves naming consistency without altering functionality.derive/tests/grammar.pest (6)
27-27: LGTM: Consistent renaming of built-in ruleThe change from
SOItosoialigns with the PR objective of renaming built-in rules to lowercase. This improves naming consistency without altering the functionality.
38-38: LGTM: Consistent renaming of built-in ruleThe change from
EOItoeoiis in line with the PR objective of renaming built-in rules to lowercase. This enhances naming consistency while preserving the existing functionality.
46-50: LGTM: Consistent renaming of ASCII-related constructsThe changes to lowercase for
ascii_digit,ascii_nonzero_digit,ascii_bin_digit,ascii_oct_digit, andascii_hex_digitare in line with the PR objective. This improves naming consistency across ASCII-related constructs while maintaining functionality.
51-53: LGTM: Consistent renaming of ASCII-related constructsThe changes to lowercase for
ascii_alpha_lower,ascii_alpha_upper, andascii_alphaalign with the PR objective. This enhances naming consistency across ASCII-related constructs while preserving functionality.
54-55: LGTM: Consistent renaming of ASCII-related constructsThe changes to lowercase for
ascii_alphanumericandasciiare in line with the PR objective. This improves naming consistency across ASCII-related constructs while maintaining functionality.
56-56: LGTM: Consistent renaming of built-in ruleThe change from
NEWLINEtonewlinealigns with the PR objective of renaming built-in rules to lowercase. This improves naming consistency without altering the functionality.generator/src/typed/output.rs (1)
Line range hint
151-164: LGTM! Improved naming consistency.The changes in the
collectmethod of theTrackerstruct align well with the PR objective of renaming built-in rules to be all lowercase. This enhances consistency across the codebase and improves readability. The modifications maintain existing functionality while standardizing the naming convention.Specifically, the following constants have been renamed:
SOItosoiEOItoeoiANYtoanyPEEKtopeekPEEK_ALLtopeek_allDROPtodropPUSHtopushPOPtopopPOP_ALLtopop_allASCII_*constants to their lowercase equivalentsThese changes contribute to a more uniform and consistent codebase.
generator/tests/expected_import_inline.rs (3)
Line range hint
70-247: New rule structures are correctly defined and consistentThe addition of structs
r#x0throughr#x4in therulesmodule enhances the grammar definitions. The helper methods and implementations adhere to the project's coding standards and correctly use lifetimes and generics.
646-647: Verify all references to renamed built-in rules are updatedThe imports have been updated to use lowercase identifiers (e.g.,
ascii_alpha,ascii_digit). Please ensure that all usages of the old uppercase identifiers have been replaced to prevent any unresolved import errors.Run the following script to check for any remaining references to the old identifiers:
#!/bin/bash # Description: Search for usages of old uppercase built-in rule names that should have been renamed. # Test: Look for any occurrences of `ASCII_` prefixed identifiers. rg 'ASCII_[A-Z_]+'
Line range hint
9-11: Ensure the newr#minimalvariant integrates without conflictsThe addition of
r#minimal(rules::r#minimal::Rule)to theRuleenum enhances modularity by nesting rules. Please verify that this new variant does not introduce any pattern matching conflicts or overlap with existing variants in the codebase.Run the following script to identify any pattern matching statements that might be affected:
| @@ -8,7 +8,7 @@ | |||
| // FIXME: some should be under pest::* | |||
| use common::base as p | |||
|
|
|||
| json = pest::SOI ~ value ~ pest::EOI | |||
| json = pest::soi ~ value ~ pest::eoi | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Remaining uppercase 'SOI' and 'EOI' found
- File:
meta/src/grammar.pest- Lines: 2, 4
- File:
generator/tests/syntax.pest- Lines: 1, 2
Please update SOI and EOI to lowercase soi and eoi in the above files to ensure consistent usage across the codebase.
🔗 Analysis chain
LGTM: Consistent renaming of built-in rules
The changes from pest::SOI to pest::soi and pest::EOI to pest::eoi are in line with the PR objective of renaming built-in rules to be all lowercase. This change enhances consistency across the codebase.
To ensure these changes are applied consistently across the project, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of lowercase 'soi' and 'eoi' across the project
# Test: Check for any remaining uppercase usage of SOI and EOI
echo "Checking for any remaining uppercase SOI and EOI usage:"
rg -i '\b(SOI|EOI)\b' --type pest
# Test: Confirm the usage of lowercase soi and eoi
echo "Confirming usage of lowercase soi and eoi:"
rg '\b(soi|eoi)\b' --type pest
Length of output: 342
Script:
#!/bin/bash
# Description: Verify consistent usage of lowercase 'soi' and 'eoi' across the project
# Find all .pest files and check for any remaining uppercase SOI and EOI usage
echo "Checking for any remaining uppercase SOI and EOI usage:"
fd --extension pest | xargs rg -i '\b(SOI|EOI)\b'
# Confirm the usage of lowercase soi and eoi
echo "Confirming usage of lowercase soi and eoi:"
fd --extension pest | xargs rg '\b(soi|eoi)\b'
Length of output: 1777
Fixes #9
For more details, open the Copilot Workspace session.
Summary by CodeRabbit