fix handling of very long sequences/choices

[Tartasprint]

Looking at code for sequences and choices it looked like sequences and choices were limited to at most 16.
Turns out the generator was already auto-generating types for longer cases, but there were two small things breaking it.
First was an off by one typo, skipping the built-ins Sequence16 and Choice16.
The other is due to some inconsistency between the calls to the generating macros and the spec of these macros.

I added a test case in derive/tests/grammar.pest

Summary by CodeRabbit

• New Features

  • Introduced macros for generating choice types and sequence types, allowing for flexible definitions ranging from 2 to 16 variants or fields.
  • Enhanced grammar rules for parsing numeric string patterns, expanding capabilities for sequences and choices.

• Bug Fixes

  • Optimized identifier generation logic in the Tracker struct, improving clarity and reducing redundancy.

• Documentation

  • Updated grammar rules in the testing framework to support new sequence and choice constructs.

[coderabbitai]

Walkthrough

This pull request introduces two macros, choice_type! and sequence_type!, which generate choice and sequence types, respectively, with a specified number of variants or fields ranging from 2 to 16. The macros create enums and structs with associated methods for parsing and handling input. Additionally, new grammar rules for numeric string sequences and choices are added to enhance parsing capabilities. Modifications to the Tracker struct in output.rs streamline identifier generation for these types, improving the clarity of the code without altering existing functionalities.

Changes

File	Change Summary
core/src/choice.rs	Added macro_rules! choice_type, multiple invocations for Choice2 to Choice16, and pub use choice_type.
core/src/sequence.rs	Added macro_rules! sequence_type, multiple invocations for Sequence2 to Sequence16, and pub use sequence_type.
derive/tests/grammar.pest	Introduced new grammar rules: longchoice_builtin, longseq_builtin, longchoice_critical, longseq_critical, longchoice_jump, longseq_jump.
generator/src/typed/output.rs	Refactored collect method in Tracker for improved identifier generation; modified length condition from len >= 16 to len > 16.

Possibly related PRs

• Remove need of pest3_core as a dependency for pest3 users #18: The changes in this PR involve the full_rule_struct macro, which is relevant to the macro choice_type! introduced in the main PR, as both pertain to the generation of types and structures in the parsing framework.

Suggested reviewers

• tomtau

Poem

In the meadow where choices bloom,
A macro hops, dispelling gloom.
With sequences long and variants bright,
Parsing dreams take joyful flight.
From two to sixteen, they dance and play,
In the world of code, they find their way! 🐇✨

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

core/src/sequence.rs (1)

Line range hint 1-3: Update documentation to include Sequence16

The documentation mentions that generated sequence types have 2 to 15 fields, but the code now includes Sequence16. Please update the documentation to reflect this addition.

core/src/choice.rs (2)

Line range hint 1-3: Update documentation to include Choice16

The documentation states that generated choice types have 2 to 15 variants, but the code actually generates Choice2 up to Choice16. Please update the documentation to accurately reflect the range of variants.

Apply this diff to correct the documentation:

-//! and generated choice types that has 2 to 15 variants,
+//! and generated choice types that have 2 to 16 variants,

---

Line range hint 1-169: Consider adding unit tests for generated choice types

While the macro generates the choice types as intended, adding unit tests for each generated ChoiceN type could help ensure their correctness and assist future maintenance.

Would you like assistance in generating sample unit tests for these types?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3b3d9b4 and 1e26c4e.

📒 Files selected for processing (4)

• core/src/choice.rs (1 hunks)
• core/src/sequence.rs (1 hunks)
• derive/tests/grammar.pest (1 hunks)
• generator/src/typed/output.rs (1 hunks)

🧰 Additional context used

🔇 Additional comments (8)

derive/tests/grammar.pest (4)

50-51: LGTM: Well-structured test cases for 16-element choices and sequences.

These rules effectively test the handling of choices and sequences with exactly 16 elements, which aligns with the PR objective of fixing handling for long sequences and choices. The rules are well-formatted and easy to read.

---

52-53: Excellent: Critical test cases for 17-element choices and sequences.

These rules are crucial for testing the PR's objective. They check the handling of choices and sequences with 17 elements, which is just beyond the previous limit of 16. This ensures that the fix works for the first case exceeding the old limit.

---

54-55: Good addition: Test cases for 19-element choices and sequences.

These rules effectively test the handling of choices and sequences with 19 elements, which is well beyond the previous limit of 16. This helps ensure the robustness of the fix for even longer sequences and choices.

Could you explain the reasoning behind choosing 19 elements for these test cases? Is there a specific significance to this number, or was it chosen arbitrarily to test beyond the critical case of 17?

---

50-55: Summary: Comprehensive test cases added for long choices and sequences.

The new rules provide a progressive set of test cases for choices and sequences with 16, 17, and 19 elements. This aligns well with the PR objectives and should provide good coverage for testing the fix for handling long sequences and choices.

To ensure these new rules are being utilized effectively:

Could you please run the following script to verify that these new rules are actually used in test cases?

This will help confirm that the new grammar rules are being properly tested in the codebase.

generator/src/typed/output.rs (1)

97-115: Improved clarity and fixed off-by-one error

The changes in this segment effectively address the PR objectives and improve the code quality:

1. The separate mapping for field, the_type, and trivia_or_getter enhances clarity and reduces redundancy.
2. Changing the condition from len >= 16 to len > 16 fixes the off-by-one error mentioned in the PR objectives, ensuring that Sequence16 and Choice16 are not skipped.
3. The more explicit generation of identifiers improves code readability.

These modifications successfully fix the handling of very long sequences/choices as intended.

core/src/sequence.rs (1)

103-103: Expose sequence_type macro publicly

The addition of pub use sequence_type; makes the sequence_type macro publicly accessible, which is appropriate for users who need to generate custom sequence types.

core/src/choice.rs (2)

103-103: Re-exporting the choice_type macro

The pub use choice_type; statement correctly re-exports the choice_type macro, making it accessible to users of the crate.

---

Line range hint 104-169: Consistency and correctness in macro invocations

The invocations of the choice_type! macro to generate Choice2 through Choice16 are consistent and correctly structured. This ensures that choice types with varying numbers of variants are properly defined.

[tomtau]

@TheVeryDarkness

[TheVeryDarkness]

In previous versions, I've added a support for sequences/choices longer than 15 by generating a new sequence/choice type, but it seems that it's not tested and has broken by re-factoring of the crate structure. Maybe let us take a glance at the test coverage results later?

@Tartasprint Thank you for mentioning and fixing it.