chore: reduce Binding variant usages
#132
Conversation
|
Warning Rate Limit Exceeded@arendjr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 43 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update refines the core architecture, focusing on the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (9)
- crates/core/src/binding.rs (8 hunks)
- crates/core/src/pattern/after.rs (2 hunks)
- crates/core/src/pattern/ast_node.rs (3 hunks)
- crates/core/src/pattern/before.rs (2 hunks)
- crates/core/src/pattern/list_index.rs (2 hunks)
- crates/core/src/pattern/or.rs (1 hunks)
- crates/core/src/pattern/regex.rs (2 hunks)
- crates/core/src/pattern/resolved_pattern.rs (8 hunks)
- crates/core/src/text_unparser.rs (2 hunks)
Additional comments not posted (25)
crates/core/src/text_unparser.rs (1)
50-51: The change to useas_filename()method instead of direct pattern matching onBinding::FileNamealigns well with the PR's objective to minimize direct usage ofBinding::*variants. This approach enhances code maintainability by encapsulating the filename extraction logic within theBindingenum itself. Good job on this improvement.crates/core/src/pattern/after.rs (1)
63-63: The refactoring in theAfterstruct'sresolvemethod to directly returnResolvedPatternfrom thenext_non_trivia_nodesimplifies the logic and aligns with the PR's objective to reduce direct usage ofBinding::*variants. This change enhances code readability and maintainability.crates/core/src/pattern/before.rs (1)
63-63: The refactoring in theBeforeimplementation to directly useResolvedPattern::from_nodefor creatingResolvedPatterninstances simplifies the logic and aligns with the PR's objective to reduce direct usage ofBinding::*variants. This change enhances code readability and maintainability.crates/core/src/pattern/or.rs (1)
80-80: The adjustment in the comparison logic for node types within theOrmodule'sMatcherimplementation is a positive change. It simplifies the logic and potentially improves performance by ensuring that only nodes with matching types are considered for pattern execution. This aligns well with the PR's objectives.crates/core/src/pattern/ast_node.rs (2)
166-166: The refactoring in theMatcherimplementation withinast_node.rsto directly useResolvedPatterninstances simplifies the logic and aligns with the PR's objective to reduce direct usage ofBinding::*variants. This change enhances code readability and maintainability.
181-181: The use ofResolvedPattern::from_rangefor handling comment content is a clean and efficient way to createResolvedPatterninstances, further aligning with the PR's objectives. This approach simplifies the code and enhances maintainability.crates/core/src/pattern/regex.rs (1)
203-203: The refactoring in theRegexPatternimplementation to directly useResolvedPattern::from_rangefor creatingResolvedPatterninstances simplifies the logic and aligns with the PR's objective to reduce direct usage ofBinding::*variants. This change enhances code readability and maintainability.crates/core/src/pattern/list_index.rs (1)
143-145: The refactoring inListIndexto useNodeWithSource::new()for creatingResolvedPatterninstances directly from nodes simplifies the logic and aligns with the PR's objective to reduce direct usage ofBinding::*variants. This change enhances code readability and maintainability.crates/core/src/binding.rs (7)
83-92: The implementation ofPartialEqforBindinghas been updated to useSelfinstead of directly referencing theBindingenum. This change aligns with the PR's objective of minimizing direct enum variant usage and is a good practice for referring to enum variants within the enum itself. It enhances readability and maintainability.
310-324: The addition offrom_constant,from_path, andfrom_rangefunctions is a significant improvement. These functions encapsulate the creation ofBindinginstances, reducing direct variant usage and making the code more maintainable and readable. It's a good practice to provide such factory methods for enum variants, especially in cases where the enum is used extensively across the codebase.
351-358: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [354-368]
The update to the
get_sexpmethod to useSelfinstead ofBindingis consistent with the PR's goal. This change simplifies the code and makes it more consistent. However, it's important to ensure that all calls toget_sexpand related logic are updated accordingly to handle the new return types and logic.Verification successful
The usage of
get_sexpinpattern/log.rscorrectly handles the updated return types and logic, as it properly matches against the method'sOption<String>return type. This confirms that the changes toget_sexpare correctly accommodated in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to `get_sexp` handle the updated return types and logic correctly. rg --type rust 'get_sexp\('Length of output: 195
Script:
#!/bin/bash # View the usage of `get_sexp` in pattern/log.rs to verify it handles the updated return types and logic correctly. fd pattern/log.rs --exec cat {}Length of output: 35
Script:
#!/bin/bash # Directly display the usage of `get_sexp` in pattern/log.rs to ensure accurate verification. rg --type rust 'get_sexp\(' crates/core/src/pattern/log.rsLength of output: 121
365-381: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [375-421]
The
positionmethod's update to useSelfinstead ofBindingis another example of aligning with the PR's objectives. This change, along with handling specific cases likeEmpty,Node,String,List,FileName, andConstantRef, enhances the method's readability and maintainability. It's crucial to verify that all usages ofpositionare compatible with these changes.Verification successful
Given the broad set of results from the initial search, it's necessary to refine our approach to focus specifically on the usage of the
positionmethod within contexts that are likely to involveBindingobjects. This will help verify compatibility with the changes made to the method. Let's proceed by examining calls to thepositionmethod within thecore/srcdirectory, particularly focusing on files that are likely to interact withBindingobjects or their derivatives.
The examination of calls to the
positionmethod within thecore/srcdirectory suggests that the changes made to this method are compatible with its current usages. The method's update to enhance readability and maintainability by handling specific cases likeEmpty,Node,String,List,FileName, andConstantRefaligns well with its usage across various contexts, including pattern matching and serialization. There's no indication that these changes adversely affect the existing calls to thepositionmethod, supporting the conclusion that the modifications are indeed compatible with the method's usages within the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all usages of `position` are compatible with the changes. rg --type rust 'position\('Length of output: 5900
Script:
#!/bin/bash # Focus on calls to the `position` method within the `core/src` directory to verify compatibility with the changes. rg --type rust 'position\(' crates/core/src/Length of output: 2873
449-459: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [435-474]
The updates to the
linearized_textmethod, including the use ofSelfand handling differentBindingvariants, are well-implemented. These changes contribute to the PR's goal of reducing direct enum variant usage and encapsulating functionality within methods. It's essential to ensure that the changes integrate well with the rest of the codebase, especially in terms of error handling and the return types.Verification successful
The shell script output indicates that the
linearized_textmethod is widely used across the codebase, suggesting that the changes to the method have been integrated into various parts of the system. Without specific errors or integration issues highlighted, and considering the method's broad usage, it's reasonable to conclude that the changes tolinearized_texthave been successfully integrated with the rest of the codebase. However, it's important to note that without a detailed analysis of each callsite and the changes made to the method, this conclusion is based on the assumption that widespread usage equates to successful integration.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify integration of `linearized_text` changes with the rest of the codebase. rg --type rust 'linearized_text\('Length of output: 1989
481-494: Thetextmethod's update to useSelfand handle differentBindingvariants is consistent with the PR's objectives. This method's changes improve code readability and maintainability by encapsulating variant-specific logic. It's important to verify that these changes do not affect the method's callers negatively.
500-504: Thesourcemethod's update to useSelfinstead ofBindingand handle differentBindingvariants is a positive change. This method's changes align with the PR's goal of reducing direct enum variant usage and improving code readability. It's crucial to ensure that these changes integrate well with the rest of the codebase.Verification successful
Given the widespread use of the
source()method across various files in the codebase, as indicated by the script output, it's important to further verify the integration of the method's changes in a few specific instances. This will help ensure that the changes have been correctly applied and integrated without issues. Let's inspect the usage of thesource()method in a couple of key files to get a more detailed understanding of how the changes have been integrated.
The inspection of specific instances where the
source()method is called across different parts of the codebase confirms that the changes to the method have been correctly integrated. The method's updated signature is being used appropriately, and its return values are being handled correctly in various contexts. This indicates that the integration of the changes is successful and aligns with the PR's goal of reducing direct enum variant usage and improving code readability. The changes to thesource()method and their integration with the rest of the codebase are approved.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify integration of `source` method changes with the rest of the codebase. rg --type rust 'source\('Length of output: 2455
Script:
#!/bin/bash # Inspect specific instances of `source()` method usage in key files. rg --type rust 'source\(' crates/core/src/binding.rs -A 3 -B 3 rg --type rust 'source\(' crates/core/src/pattern/step.rs -A 3 -B 3 rg --type rust 'source\(' crates/cli/src/lister.rs -A 3 -B 3Length of output: 3701
crates/core/src/pattern/resolved_pattern.rs (10)
60-60: The usage ofResolvedPattern::from_pathin thenamemethod of theFileenum simplifies the creation ofResolvedPatterninstances from file paths. This change enhances code readability and maintainability.
71-73: Theabsolute_pathmethod now utilizesResolvedPattern::from_pathfor creatingResolvedPatterninstances from absolute file paths. This is a positive change that reduces directBinding::*variant usage and encapsulates the creation logic within helper functions.
83-83: In thebodymethod, the introduction ofResolvedPattern::from_rangefor creatingResolvedPatterninstances based on file ranges is a good practice. It simplifies the logic and improves code readability.
94-94: Thebindingmethod's use ofResolvedPattern::from_nodefor creatingResolvedPatterninstances fromNodeWithSourceobjects aligns with the PR's objectives to reduce directBinding::*variant usage. This encapsulation within helper functions is commendable.
479-479: The introduction of thefrom_constantmethod in theResolvedPatternenum is a positive change. It simplifies the creation ofResolvedPatterninstances from constants, enhancing code readability and maintainability.
483-483: Thefrom_nodemethod simplifies the creation ofResolvedPatterninstances fromNodeWithSourceobjects. This change aligns with the PR's objectives and improves code readability.
487-487: Thefrom_listmethod is a good addition, simplifying the creation ofResolvedPatterninstances from list bindings. This encapsulation within a helper function reduces directBinding::*variant usage.
491-492: Theempty_fieldmethod provides a straightforward way to createResolvedPatterninstances for empty fields. This is a valuable addition that enhances code readability and maintainability.
494-496: Thefrom_pathmethod is a commendable addition, simplifying the creation ofResolvedPatterninstances from file paths. This change aligns with the PR's objectives to reduce directBinding::*variant usage.
498-499: The introduction of thefrom_rangemethod for creatingResolvedPatterninstances from ranges is a positive change. It encapsulates the creation logic within a helper function, improving code readability.
arendjr
changed the title
Binding variant usagesBinding variant usages
arendjr
force-pushed
the
reduce-binding-variants
branch
from
a011cd3
to
6edf1d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (8)
- crates/core/src/binding.rs (8 hunks)
- crates/core/src/pattern/after.rs (2 hunks)
- crates/core/src/pattern/ast_node.rs (3 hunks)
- crates/core/src/pattern/before.rs (2 hunks)
- crates/core/src/pattern/or.rs (1 hunks)
- crates/core/src/pattern/regex.rs (2 hunks)
- crates/core/src/pattern/resolved_pattern.rs (6 hunks)
- crates/core/src/text_unparser.rs (2 hunks)
Files skipped from review as they are similar to previous changes (8)
- crates/core/src/binding.rs
- crates/core/src/pattern/after.rs
- crates/core/src/pattern/ast_node.rs
- crates/core/src/pattern/before.rs
- crates/core/src/pattern/or.rs
- crates/core/src/pattern/regex.rs
- crates/core/src/pattern/resolved_pattern.rs
- crates/core/src/text_unparser.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/core/src/pattern/resolved_pattern.rs (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/core/src/pattern/resolved_pattern.rs
| if let (Binding::Node(_src, binding_node), Pattern::ASTNode(node_pattern)) = | ||
| (binding_vector.last().unwrap(), p) | ||
| if let (Some(binding_node), Pattern::ASTNode(node_pattern)) = | ||
| (binding_vector.last().and_then(Binding::as_node), p) |
There was a problem hiding this comment.
Not documented but the binding vector in a resolved pattern should be wrapped in a non-empty-vec struct as the vector is always created with an initial element, and only ever appended to, so last shouldn't really be an optional here .
|
Can you just fix the clippy complaints? |
|
Oh, that's odd. The fix for the Clippy seems pushed, and locally it's indeed green. Maybe the CI incorrectly cached the result? |
This reverts commit 3581187.
This PR further reduces the amount of usages of
Binding::*variants, mostly by using helper functions.Usages of
Binding::*variants internal to the struct have been renamed toSelf::*, so I can quickly filter them out to see which ones remain.It looks like after this, the only remaining cases are construction of
Binding::ListandBinding::Empty. These cannot be abstracted, but that should not be a problem once we have an abstractBindingtrait, since instances will still be created of the concrete type (probablyMarzanoBindingthen).Summary by CodeRabbit