-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: use NodeWithContainer
in ResolvedPattern::from_node()
+ more Binding::list_items()
usage
#103
chore: use NodeWithContainer
in ResolvedPattern::from_node()
+ more Binding::list_items()
usage
#103
Conversation
WalkthroughThe overarching change across these updates focuses on simplifying argument passing and refining method calls within the codebase, particularly around 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (10)
- crates/core/src/pattern/after.rs (1 hunks)
- crates/core/src/pattern/ast_node.rs (1 hunks)
- crates/core/src/pattern/before.rs (1 hunks)
- crates/core/src/pattern/built_in_functions.rs (2 hunks)
- crates/core/src/pattern/contains.rs (3 hunks)
- crates/core/src/pattern/every.rs (1 hunks)
- crates/core/src/pattern/list.rs (1 hunks)
- crates/core/src/pattern/resolved_pattern.rs (2 hunks)
- crates/core/src/pattern/some.rs (1 hunks)
- crates/core/src/pattern/within.rs (2 hunks)
Additional comments (12)
crates/core/src/pattern/within.rs (1)
- 91-91: The change to use
NodeWithSource::new(n, node.source)
for creating aNodeWithSource
instance and passing it toResolvedPattern::from_node
is correctly implemented and aligns with the PR objectives to simplify argument passing and improve modularity. Good job on this improvement.crates/core/src/pattern/after.rs (1)
- 106-106: The simplification of the argument passed to
ResolvedPattern::from_node
is correctly implemented, making the code more concise and easier to understand. This aligns well with the PR objectives.crates/core/src/pattern/before.rs (1)
- 106-106: The simplification of the input to
ResolvedPattern::from_node
by removing unnecessary details is correctly implemented, enhancing code readability and maintainability. This aligns well with the PR objectives.crates/core/src/pattern/every.rs (1)
- 76-76: The simplification of the argument passed to
ResolvedPattern::from_node
within a loop is correctly implemented, making the code more concise and easier to understand. This aligns well with the PR objectives.crates/core/src/pattern/some.rs (1)
- 77-77: The simplification of the argument passing for pattern resolution in the
Some
module'sMatcher
implementation is correctly implemented, enhancing code readability and maintainability. This aligns well with the PR objectives.crates/core/src/pattern/ast_node.rs (1)
- 206-206: The update to the argument passed to
ResolvedPattern::from_node
usingNodeWithSource::new(child, source)
is correctly implemented, simplifying the code and aligning well with the PR objectives.crates/core/src/pattern/list.rs (1)
- 129-130: The simplification of the mapping of
list_items
tochildren
by directly usingResolvedPattern::from_node
andCow::Owned
in themap
operations is correctly implemented, making the code more concise and easier to understand. This aligns well with the PR objectives.crates/core/src/pattern/contains.rs (1)
- 89-89: The use of
NodeWithSource::new(node, src)
in theexecute_until
function is correctly implemented, simplifying the code and making it more modular. This aligns well with the PR objectives.crates/core/src/pattern/built_in_functions.rs (2)
- 405-418: The refactoring of
distinct_fn
to directly handle list items and simplify the logic is a positive change. Ensure the logic correctly identifies and handles unique list items.- 449-458: The refactoring of
shuffle_fn
to directly handle list items and simplify the logic is a positive change. Ensure the logic correctly shuffles the list items.crates/core/src/pattern/resolved_pattern.rs (2)
- 20-22: The import of
NodeWithSource
aligns with the PR objectives, aiming for a cleaner and more modular architecture. Ensure its usage is consistent across the file.- 483-484: The modification of the
from_node
function to acceptNodeWithSource
instead of separate parameters is a positive change, enhancing modularity. Ensure the logic within the function correctly handles theNodeWithSource
type.
&node.node, | ||
node.source, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
execute_until
could take a node with src instead?
With this PR I converted a few more places to use
Binding::list_items()
.It also became easier to use
NodeWithContainer
inResolvedPattern::from_node()
, which is a small step towards making sureResolvedPattern
also doesn't depend onNode
directly anymore. In a later follow-upNodeWithContainer
will probably be swapped forimpl AstNode
.Summary by CodeRabbit
ResolvedPattern::from_node
across multiple modules for more consistent and simplified code.