refactor: separate pattern factory from patterns #164
Conversation
WalkthroughThe changes encompass a comprehensive reorganization of import statements across various files in the codebase, focusing on enhancing clarity and maintainability. Key adjustments include the introduction of new modules like 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 (
|
|
The approach is reasonable to me, especially since it means we can eventually end up with a hard split between the compiler and the executor/"runtime." Two naming/structural ideas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (53)
- crates/cli/src/analyze.rs (2 hunks)
- crates/cli/src/commands/apply_pattern.rs (1 hunks)
- crates/cli/src/result_formatting.rs (1 hunks)
- crates/core/src/lib.rs (1 hunks)
- crates/core/src/pattern.rs (2 hunks)
- crates/core/src/pattern/accessor.rs (1 hunks)
- crates/core/src/pattern/accumulate.rs (1 hunks)
- crates/core/src/pattern/add.rs (1 hunks)
- crates/core/src/pattern/after.rs (1 hunks)
- crates/core/src/pattern/analysis.rs (1 hunks)
- crates/core/src/pattern/and.rs (1 hunks)
- crates/core/src/pattern/any.rs (1 hunks)
- crates/core/src/pattern/assignment.rs (1 hunks)
- crates/core/src/pattern/ast_node.rs (1 hunks)
- crates/core/src/pattern/before.rs (1 hunks)
- crates/core/src/pattern/bubble.rs (1 hunks)
- crates/core/src/pattern/call.rs (2 hunks)
- crates/core/src/pattern/constants.rs (1 hunks)
- crates/core/src/pattern/container.rs (2 hunks)
- crates/core/src/pattern/contains.rs (1 hunks)
- crates/core/src/pattern/divide.rs (1 hunks)
- crates/core/src/pattern/dynamic_snippet.rs (1 hunks)
- crates/core/src/pattern/equal.rs (1 hunks)
- crates/core/src/pattern/every.rs (1 hunks)
- crates/core/src/pattern/function_definition.rs (1 hunks)
- crates/core/src/pattern/if.rs (1 hunks)
- crates/core/src/pattern/includes.rs (1 hunks)
- crates/core/src/pattern/like.rs (1 hunks)
- crates/core/src/pattern/limit.rs (1 hunks)
- crates/core/src/pattern/list.rs (1 hunks)
- crates/core/src/pattern/list_index.rs (1 hunks)
- crates/core/src/pattern/log.rs (1 hunks)
- crates/core/src/pattern/map.rs (1 hunks)
- crates/core/src/pattern/match.rs (1 hunks)
- crates/core/src/pattern/maybe.rs (1 hunks)
- crates/core/src/pattern/modulo.rs (1 hunks)
- crates/core/src/pattern/multiply.rs (1 hunks)
- crates/core/src/pattern/not.rs (2 hunks)
- crates/core/src/pattern/or.rs (1 hunks)
- crates/core/src/pattern/pattern_definition.rs (1 hunks)
- crates/core/src/pattern/patterns.rs (3 hunks)
- crates/core/src/pattern/predicate_definition.rs (1 hunks)
- crates/core/src/pattern/predicate_return.rs (1 hunks)
- crates/core/src/pattern/predicates.rs (2 hunks)
- crates/core/src/pattern/regex.rs (1 hunks)
- crates/core/src/pattern/rewrite.rs (2 hunks)
- crates/core/src/pattern/sequential.rs (3 hunks)
- crates/core/src/pattern/some.rs (1 hunks)
- crates/core/src/pattern/state.rs (1 hunks)
- crates/core/src/pattern/step.rs (1 hunks)
- crates/core/src/pattern/subtract.rs (1 hunks)
- crates/core/src/pattern/variable.rs (2 hunks)
- crates/core/src/pattern/where.rs (2 hunks)
Files not processed due to max files limit (12)
- crates/core/src/pattern/within.rs
- crates/core/src/pattern_factory/accessor_factory.rs
- crates/core/src/pattern_factory/auto_wrap.rs
- crates/core/src/pattern_factory/compiler.rs
- crates/core/src/pattern_factory/mod.rs
- crates/core/src/pattern_factory/step_factory.rs
- crates/core/src/test.rs
- crates/lsp/src/apply.rs
- crates/lsp/src/check.rs
- crates/lsp/src/search.rs
- crates/lsp/src/testing.rs
- crates/wasm-bindings/src/match_pattern.rs
Files skipped from review due to trivial changes (5)
- crates/cli/src/commands/apply_pattern.rs
- crates/core/src/pattern/analysis.rs
- crates/core/src/pattern/assignment.rs
- crates/core/src/pattern/constants.rs
- crates/core/src/pattern/every.rs
Additional Context Used
Learnings (1)
crates/core/src/pattern/variable.rs (1)
User: urbit-pilled" PR: getgrit/gritql#52 File: crates/core/src/pattern/variable.rs:0-0 Timestamp: 2024-03-25T23:06:45.275Z Learning: The prefix `^GLOBAL_` is used within GritQL, not PHP, to distinguish global variables.
Additional comments not posted (60)
crates/core/src/lib.rs (1)
14-14: Consider adding documentation for thepattern_factorymodule to explain its purpose and usage.crates/core/src/pattern/predicate_return.rs (1)
8-8: Ensure that movingCompilationContexttopattern_factorydoes not introduce circular dependencies.crates/core/src/pattern/equal.rs (1)
7-7: Ensure that movingCompilationContexttopattern_factorydoes not introduce circular dependencies.crates/core/src/pattern/add.rs (1)
7-7: Ensure that movingCompilationContexttopattern_factorydoes not introduce circular dependencies.crates/core/src/pattern/divide.rs (1)
7-7: Ensure that movingCompilationContexttopattern_factorydoes not introduce circular dependencies.crates/core/src/pattern/multiply.rs (1)
7-7: Ensure that movingCompilationContexttopattern_factorydoes not introduce circular dependencies.crates/core/src/pattern/subtract.rs (1)
7-7: Ensure that movingCompilationContexttopattern_factorydoes not introduce circular dependencies.crates/core/src/pattern/modulo.rs (1)
7-7: Ensure that movingCompilationContexttopattern_factorydoes not introduce circular dependencies.crates/core/src/pattern/like.rs (1)
8-8: The reorganization of imports, specifically movingCompilationContextunderpattern_factory, aligns well with the PR's objective to separate concerns. This change enhances modularity and maintainability.crates/core/src/pattern/map.rs (1)
6-6: The reorganization of imports inmap.rs, particularly movingCompilationContexttopattern_factory, is a positive change that supports the PR's goal of modularizing the codebase.crates/core/src/pattern/limit.rs (1)
7-7: The reorganization of imports inlimit.rs, movingCompilationContexttopattern_factory, supports the PR's objective of reducing direct dependencies and enhancing code modularity.crates/core/src/pattern/predicate_definition.rs (1)
9-9: The reorganization of imports inpredicate_definition.rs, particularly adjusting the paths forContextandCompilationContext, aligns with the PR's objective to enhance code modularity and clarity.crates/core/src/pattern/sequential.rs (1)
11-11: The reorganization of imports insequential.rs, including the use ofstep_from_nodefrompattern_factory, aligns with the PR's objective to separate pattern creation logic, enhancing code modularity and clarity.crates/core/src/pattern/after.rs (1)
9-9: The reorganization of imports inafter.rs, movingCompilationContextanddebugtopattern_factory, supports the PR's objective of reducing direct dependencies and enhancing code modularity.crates/core/src/pattern/before.rs (1)
9-9: The reorganization of imports inbefore.rs, adjusting the import statements forCompilationContext, aligns with the PR's objective to enhance code modularity and clarity.crates/core/src/pattern/maybe.rs (1)
9-9: The reorganization of imports inmaybe.rs, movingCompilationContexttopattern_factory, supports the PR's objective of reducing direct dependencies and enhancing code modularity.crates/core/src/pattern/pattern_definition.rs (1)
8-8: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.crates/core/src/pattern/bubble.rs (1)
5-5: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.crates/core/src/pattern/container.rs (2)
9-9: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.
61-61: The change to useaccessor_from_nodefor creatingAccessorinstances is a positive step towards improving modularity and maintainability.crates/core/src/pattern/and.rs (1)
9-9: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.crates/core/src/pattern/includes.rs (1)
7-7: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.crates/core/src/pattern/some.rs (1)
7-7: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.crates/core/src/pattern/not.rs (1)
10-10: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.crates/core/src/pattern/any.rs (1)
9-9: Ensure to check for any potential issues caused by the reorganization of imports, especially regarding circular dependencies.crates/core/src/pattern/accessor.rs (2)
33-36: Thenewmethod forAccessoris straightforward and correctly implemented.
39-40: Ensure proper error handling for theVariablevariant inget_keywhen the variable's text cannot be retrieved.Verification successful
The
Variable::text(state)method implementation invariable.rshas been located. This method returns aResult<Cow<'a, str>>, indicating that it is designed to handle errors properly by returning them instead of causing a panic. The method retrieves the text representation of a variable's value from the state, and if any step in this process fails (e.g., if the variable's value is not found), it will result in an error being returned. This aligns with the requirement for proper error handling mentioned in the review comment.Given this information, it can be concluded that the
Variable::text(state)method does indeed handle errors appropriately, as it utilizes Rust'sResulttype for error handling, which prevents the method from panicking due to unhandled errors.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that Variable::text(state) properly handles errors and does not panic.Length of output: 0
Script:
#!/bin/bash # Locate the implementation of the Variable::text method to verify error handling fd "accessor.rs" --exec cat {}Length of output: 4963
Script:
#!/bin/bash # Locate the implementation of the Variable::text method to verify error handling fd "variable.rs" --exec cat {}Length of output: 13087
crates/core/src/pattern/log.rs (2)
3-3: The reorganization of imports to includePatternfrom thepatternsmodule is appropriate for the file's context.
8-8: The addition ofCompilationContextfrompattern_factoryis a good practice, ensuring that the context is sourced from the appropriate module following the refactor.crates/core/src/pattern/or.rs (1)
9-9: The adjustment in the import path forCompilationContextaligns with the refactor's goal of better organizing the codebase.crates/core/src/pattern/dynamic_snippet.rs (3)
4-4: The usage of constantsDEFAULT_FILE_NAMEandGLOBAL_VARS_SCOPE_INDEXis clear and well-defined.
9-9: The reorganization of imports to includeVariablefrom thevariablemodule is appropriate and improves the file's clarity.
12-12: The addition ofsplit_snippetfrom thecratelevel is a good practice, ensuring that the function is sourced from the appropriate module.crates/core/src/pattern/if.rs (1)
9-9: The adjustment in the import path forCompilationContextaligns with the refactor's goal of better organizing the codebase.crates/core/src/pattern/match.rs (1)
9-9: The adjustment in the import path forCompilationContextaligns with the refactor's goal of better organizing the codebase.crates/core/src/pattern/where.rs (1)
12-13: The adjustment in the import path forCompilationContextaligns with the refactor's goal of better organizing the codebase.crates/core/src/pattern/ast_node.rs (1)
9-9: The reorganization of imports to includeCompilationContextfrom thepattern_factorymodule is appropriate for the file's context.crates/core/src/pattern/step.rs (2)
1-18: The reorganization of imports improves readability and maintainability.
18-18: Ensure the removal ofwrap_pattern_in_before_and_after_each_filefrom thefrom_nodemethod does not negatively impact the pattern handling logic.Verification successful
The removal of
wrap_pattern_in_before_and_after_each_filefrom thefrom_nodemethod does not seem to negatively impact the pattern handling logic, as the function is still actively used within the codebase in other contexts. This suggests that the pattern handling logic may have been refactored or redistributed rather than diminished. Therefore, the initial concern appears to be mitigated by the continued use of this function in other parts of the system.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the removal of wrap_pattern_in_before_and_after_each_file affects pattern handling grep -r "wrap_pattern_in_before_and_after_each_file" .Length of output: 596
crates/core/src/pattern/list.rs (1)
8-8: The reorganization of imports, including movingCompilationContext, aligns with the PR's objective to restructure dependencies for better modularity.crates/core/src/pattern/predicates.rs (1)
21-21: The changes in import statements forContextandCompilationContextsupport the PR's goal of better separating concerns within the codebase.crates/core/src/pattern/regex.rs (1)
8-8: The reorganization of imports, including the adjustment of paths forCompilationContextandContext, enhances the modularity of the codebase.crates/core/src/pattern/accumulate.rs (1)
1-15: The reorganization of imports undersuperand the adjustment of the order of imported modules enhance the clarity and consistency of the codebase.crates/core/src/pattern/function_definition.rs (1)
10-10: The addition of new imports, includingbinding::Constant, aligns with the potential expansion of functionality within the module.crates/core/src/pattern/list_index.rs (1)
10-15: The reorganization of imports, including those related to bindings and context, supports the PR's goal of modularizing the codebase and clarifying dependencies.crates/core/src/pattern/state.rs (2)
1-2: The reorganization of imports and the adjustment of theMATCH_VARimport enhance clarity and maintainability.
7-7: MovingEffectandFileOwnerdeclarations within the import block improves readability by grouping related components together.crates/core/src/pattern/contains.rs (1)
7-7: The change in the import statement correctly reflects the new location ofCompilationContextwithin thepattern_factory::compilermodule.crates/core/src/pattern/rewrite.rs (1)
11-11: The change in the import statement correctly reflects the new location ofCompilationContextwithin thepattern_factory::compilermodule.crates/cli/src/analyze.rs (1)
22-25: The changes in the import statements correctly reflect the adjustments in the codebase forPositionandCompilationResult.crates/core/src/pattern/variable.rs (1)
2-2: The change in the import statement correctly reflects the new location ofGLOBAL_VARS_SCOPE_INDEXwithin theconstantsmodule.crates/cli/src/result_formatting.rs (1)
11-11: The change in the import statement correctly reflects the new location ofDEFAULT_FILE_NAMEwithin theconstantsmodule.crates/core/src/pattern/call.rs (1)
13-13: Ensure thatCompilationContextis correctly used throughout this file after its import path change.crates/core/src/pattern.rs (3)
1-66: Review the necessity of making all these modules public to ensure that internal implementation details are not inadvertently exposed.
1-97: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [67-108]
Verify the correctness of the new import paths to ensure that no imports are missing or unnecessarily included.
17-17: Review the responsibilities and definitions of the new modulesconstants,paths, andruntimeto ensure clarity and maintainability.Also applies to: 45-45, 94-94
crates/core/src/pattern/patterns.rs (4)
1-56: Ensure that all imported modules are used withinpatterns.rsto avoid unnecessary dependencies and compile-time overhead. Consider organizing imports in a consistent order: standard library, external crates, and then internal modules for readability.
1-60: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [54-62]
Consider adding documentation comments to the
MatcherandNametraits to explain their purpose and usage within the pattern matching system. This will improve code maintainability and readability for future contributors.
65-70: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [67-92]
Ensure that the
Patternenum covers all necessary pattern types for the application's pattern matching needs. Consider adding documentation comments to each variant to explain their specific role and usage within the system.
931-931: Verify that theaccessor_from_nodefunction correctly handles all edge cases and errors. Since it's a critical part of creatingAccessorpatterns, thorough testing and error handling are essential to ensure robustness.
| pub enum AccessorMap { | ||
| Container(Container), | ||
| Map(GritMap), | ||
| } |
There was a problem hiding this comment.
Consider adding documentation comments for the AccessorMap enum to explain its purpose and usage.
+ /// Represents the mapping for an accessor, which can be either a container or a map.
pub enum AccessorMap {
Container(Container),
Map(GritMap),
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub enum AccessorMap { | |
| Container(Container), | |
| Map(GritMap), | |
| } | |
| /// Represents the mapping for an accessor, which can be either a container or a map. | |
| pub enum AccessorMap { | |
| Container(Container), | |
| Map(GritMap), | |
| } |
| pub map: AccessorMap, | ||
| pub key: AccessorKey, |
There was a problem hiding this comment.
Rename AccessorKey to Key to maintain simplicity and directness unless there's a specific reason for the more verbose name.
- pub key: AccessorKey,
+ pub key: Key,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub map: AccessorMap, | |
| pub key: AccessorKey, | |
| pub map: AccessorMap, | |
| pub key: Key, |
| pub enum AccessorKey { | ||
| String(String), | ||
| Variable(Variable), | ||
| } |
There was a problem hiding this comment.
The AccessorKey enum is well-defined. Consider adding documentation comments to explain the purpose of each variant.
+ /// Defines the key types for an accessor, supporting both string literals and variables.
pub enum AccessorKey {
String(String),
Variable(Variable),
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub enum AccessorKey { | |
| String(String), | |
| Variable(Variable), | |
| } | |
| /// Defines the key types for an accessor, supporting both string literals and variables. | |
| pub enum AccessorKey { | |
| String(String), | |
| Variable(Variable), | |
| } |
| pub fn new(map: AccessorMap, key: AccessorKey) -> Self { | ||
| Self { map, key } | ||
| } | ||
|
|
There was a problem hiding this comment.
Refactor the Accessor struct to use a more idiomatic Rust approach for the get_key method, leveraging Rust's pattern matching capabilities.
impl Accessor {
pub fn new(map: AccessorMap, key: AccessorKey) -> Self {
Self { map, key }
}
- fn get_key<'a>(&'a self, state: &State<'a>) -> Result<Cow<'a, str>> {
- match &self.key {
- AccessorKey::String(s) => Ok(Cow::Borrowed(s)),
- AccessorKey::Variable(v) => v.text(state),
- }
- }
+ fn get_key<'a>(&'a self, state: &State<'a>) -> Result<Cow<'a, str>, anyhow::Error> {
+ let key_text = match &self.key {
+ AccessorKey::String(s) => Cow::Borrowed(s.as_str()),
+ AccessorKey::Variable(v) => Cow::Owned(v.text(state)?.to_string()),
+ };
+ Ok(key_text)
+ }
}Also applies to: 39-40
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub fn new(map: AccessorMap, key: AccessorKey) -> Self { | |
| Self { map, key } | |
| } | |
| pub fn new(map: AccessorMap, key: AccessorKey) -> Self { | |
| Self { map, key } | |
| } | |
| fn get_key<'a>(&'a self, state: &State<'a>) -> Result<Cow<'a, str>, anyhow::Error> { | |
| let key_text = match &self.key { | |
| AccessorKey::String(s) => Cow::Borrowed(s.as_str()), | |
| AccessorKey::Variable(v) => Cow::Owned(v.text(state)?.to_string()), | |
| }; | |
| Ok(key_text) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (57)
- crates/cli/src/analyze.rs (2 hunks)
- crates/cli/src/commands/apply_pattern.rs (1 hunks)
- crates/core/src/lib.rs (1 hunks)
- crates/core/src/pattern/accumulate.rs (1 hunks)
- crates/core/src/pattern/add.rs (1 hunks)
- crates/core/src/pattern/after.rs (1 hunks)
- crates/core/src/pattern/analysis.rs (1 hunks)
- crates/core/src/pattern/and.rs (1 hunks)
- crates/core/src/pattern/any.rs (1 hunks)
- crates/core/src/pattern/assignment.rs (1 hunks)
- crates/core/src/pattern/ast_node.rs (1 hunks)
- crates/core/src/pattern/before.rs (1 hunks)
- crates/core/src/pattern/bubble.rs (1 hunks)
- crates/core/src/pattern/call.rs (2 hunks)
- crates/core/src/pattern/container.rs (2 hunks)
- crates/core/src/pattern/contains.rs (1 hunks)
- crates/core/src/pattern/divide.rs (1 hunks)
- crates/core/src/pattern/equal.rs (1 hunks)
- crates/core/src/pattern/every.rs (1 hunks)
- crates/core/src/pattern/function_definition.rs (1 hunks)
- crates/core/src/pattern/if.rs (1 hunks)
- crates/core/src/pattern/includes.rs (1 hunks)
- crates/core/src/pattern/like.rs (1 hunks)
- crates/core/src/pattern/limit.rs (1 hunks)
- crates/core/src/pattern/list.rs (1 hunks)
- crates/core/src/pattern/list_index.rs (1 hunks)
- crates/core/src/pattern/log.rs (1 hunks)
- crates/core/src/pattern/map.rs (1 hunks)
- crates/core/src/pattern/match.rs (1 hunks)
- crates/core/src/pattern/maybe.rs (1 hunks)
- crates/core/src/pattern/modulo.rs (1 hunks)
- crates/core/src/pattern/multiply.rs (1 hunks)
- crates/core/src/pattern/not.rs (2 hunks)
- crates/core/src/pattern/or.rs (1 hunks)
- crates/core/src/pattern/pattern_definition.rs (1 hunks)
- crates/core/src/pattern/patterns.rs (3 hunks)
- crates/core/src/pattern/predicate_definition.rs (1 hunks)
- crates/core/src/pattern/predicate_return.rs (1 hunks)
- crates/core/src/pattern/predicates.rs (2 hunks)
- crates/core/src/pattern/regex.rs (1 hunks)
- crates/core/src/pattern/rewrite.rs (2 hunks)
- crates/core/src/pattern/sequential.rs (3 hunks)
- crates/core/src/pattern/some.rs (1 hunks)
- crates/core/src/pattern/step.rs (1 hunks)
- crates/core/src/pattern/subtract.rs (1 hunks)
- crates/core/src/pattern/where.rs (2 hunks)
- crates/core/src/pattern/within.rs (1 hunks)
- crates/core/src/pattern_compiler/accessor_compiler.rs (1 hunks)
- crates/core/src/pattern_compiler/auto_wrap.rs (2 hunks)
- crates/core/src/pattern_compiler/compiler.rs (2 hunks)
- crates/core/src/pattern_compiler/mod.rs (1 hunks)
- crates/core/src/pattern_compiler/node_compiler.rs (1 hunks)
- crates/core/src/pattern_compiler/step_compiler.rs (1 hunks)
- crates/core/src/test.rs (2 hunks)
- crates/lsp/src/apply.rs (1 hunks)
- crates/lsp/src/check.rs (1 hunks)
- crates/lsp/src/search.rs (1 hunks)
Files not processed due to max files limit (2)
- crates/lsp/src/testing.rs
- crates/wasm-bindings/src/match_pattern.rs
Files skipped from review as they are similar to previous changes (46)
- crates/cli/src/analyze.rs
- crates/cli/src/commands/apply_pattern.rs
- crates/core/src/lib.rs
- crates/core/src/pattern/accumulate.rs
- crates/core/src/pattern/add.rs
- crates/core/src/pattern/after.rs
- crates/core/src/pattern/analysis.rs
- crates/core/src/pattern/and.rs
- crates/core/src/pattern/any.rs
- crates/core/src/pattern/assignment.rs
- crates/core/src/pattern/ast_node.rs
- crates/core/src/pattern/before.rs
- crates/core/src/pattern/bubble.rs
- crates/core/src/pattern/call.rs
- crates/core/src/pattern/container.rs
- crates/core/src/pattern/contains.rs
- crates/core/src/pattern/divide.rs
- crates/core/src/pattern/equal.rs
- crates/core/src/pattern/every.rs
- crates/core/src/pattern/function_definition.rs
- crates/core/src/pattern/if.rs
- crates/core/src/pattern/includes.rs
- crates/core/src/pattern/like.rs
- crates/core/src/pattern/limit.rs
- crates/core/src/pattern/list.rs
- crates/core/src/pattern/list_index.rs
- crates/core/src/pattern/log.rs
- crates/core/src/pattern/map.rs
- crates/core/src/pattern/match.rs
- crates/core/src/pattern/maybe.rs
- crates/core/src/pattern/modulo.rs
- crates/core/src/pattern/multiply.rs
- crates/core/src/pattern/not.rs
- crates/core/src/pattern/or.rs
- crates/core/src/pattern/pattern_definition.rs
- crates/core/src/pattern/patterns.rs
- crates/core/src/pattern/predicate_definition.rs
- crates/core/src/pattern/predicate_return.rs
- crates/core/src/pattern/predicates.rs
- crates/core/src/pattern/regex.rs
- crates/core/src/pattern/rewrite.rs
- crates/core/src/pattern/sequential.rs
- crates/core/src/pattern/some.rs
- crates/core/src/pattern/step.rs
- crates/core/src/pattern/subtract.rs
- crates/core/src/pattern/where.rs
Additional comments not posted (19)
crates/core/src/pattern_compiler/mod.rs (1)
1-9: The module structure and visibility declarations inmod.rsare well-organized and follow Rust best practices.crates/core/src/pattern_compiler/node_compiler.rs (1)
1-20: TheNodeCompilertrait is well-designed, providing a clear and flexible contract for compiler implementations. It correctly uses Rust features like generics and mutable references to achieve its functionality.crates/lsp/src/search.rs (2)
1-1: The adjustments to import statements insearch.rsalign with the PR's objective of reorganizing the codebase for better modularity and clarity.
2-2: The logic within thesearch_queryfunction is well-structured, correctly handling asynchronous operations and errors, which is crucial for the functionality it provides.crates/core/src/pattern_compiler/accessor_compiler.rs (1)
1-73: The implementation of theNodeCompilertrait forAccessorCompileris well-done, with clear logic and appropriate error handling. It effectively demonstrates the use of Rust's pattern matching and error handling idioms.crates/core/src/pattern/within.rs (2)
1-1: The adjustments to import statements inwithin.rsalign with the PR's objective of reorganizing the codebase for better modularity and clarity.
7-7: The implementations of theNameandMatchertraits forWithinare logically sound and demonstrate a good use of Rust idioms, contributing to the pattern's functionality.crates/lsp/src/apply.rs (2)
3-4: The adjustments to import statements inapply.rsalign with the PR's objective of reorganizing the codebase for better modularity and clarity.
4-4: The logic within the functions for applying patterns is well-structured, correctly handling asynchronous operations and errors, which is crucial for the functionality they provide.crates/lsp/src/check.rs (2)
12-13: The adjustments to import statements incheck.rsalign with the PR's objective of reorganizing the codebase for better modularity and clarity.
13-13: The logic within the functions for checking documents against patterns is well-structured, correctly handling asynchronous operations and errors, which is crucial for the functionality they provide.crates/core/src/pattern_compiler/step_compiler.rs (1)
1-132: The implementation of theNodeCompilertrait forStepCompileris well-executed, with clear logic, appropriate error handling, and a nuanced approach to compiling step patterns. It effectively demonstrates the use of Rust's pattern matching and error handling idioms.crates/core/src/pattern_compiler/auto_wrap.rs (3)
27-27: Reorganizing imports for clarity and consistency is good practice.
30-30: Verify that the change in visibility ofauto_wrap_patterntopub(super)aligns with the intended encapsulation and usage within the module hierarchy.
1-16: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-22]
Ensure that the updates to pattern component references like
And,PrAnd,PrOr, andStringConstantare consistent with the new module structure and do not introduce any unintended behavior.crates/core/src/pattern_compiler/compiler.rs (3)
1-24: Reorganizing and adding new imports to align with the updated module structure is good practice. Ensure that these changes do not introduce unnecessary dependencies.
5-9: Confirm that the movement of constants and updates to analysis-related modules enhance clarity, maintainability, and align with the refactor's goals without causing regressions.
37-37: Assess if any visibility changes for structs align with their intended use within the new module structure and do not restrict necessary access.crates/core/src/test.rs (1)
1-1: The import statement addition aligns well with the PR's objectives of reorganizing code for better separation of concerns.
|
@arendjr Please request review when this is ready (or mark as draft). |
|
I don’t think GitHub lets me request reviews by handle on this repo (though I can request re-review when someone’s left one already), but it’s ready for review, @morgante. |
Here's one that might be a little more controversial: I anticipate that all the
PatternX::from_node()methods will always remain tied to TreeSitter, because they construct patterns from concreteNodetypes. I could maybe simplify them a little by usingNodeWithSource, but I'm afraid it will be infeasible to make them abstract enough to useimpl AstNodelater on.So instead, I propose to split the factory functions from the patterns and create a
pattern_factorymodule for them, which is what this PR does. I've only created factory functions forAccessorandStepso far, and I've also moved the compiler (which directly uses TreeSitter of course) into thepattern_factorydirectory. I've movedauto_wrap.rstoo, since it's only used bycompiler.rsandstep.rs.This creates a bit of a circular dependency between the
patternmodule and thepattern_factorymodule, but that should be temporary. If we agree on this approach, I'll continue moving the functions out of the other patterns untilpatterndoesn't depend onpattern_factoryanymore. And of course, the end goal is still forpatternto not depend on TreeSitter at all anymore, so eventually it can be moved wholesale into the futuregrit-core-patternscrate.PS.: This PR appears large, but it's really almost all updated import statements and a little bit of moving around of code.
Summary by CodeRabbit
New Features
pattern_factoryto enhance pattern handling capabilities.Refactor
Accessorstruct and renamedKeytoAccessorKeyfor better code readability.superandpattern_factoryto maintain consistency in the codebase.Stepstruct, removing unnecessary pattern checks.Style