Skip to content
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

refactor: separate pattern factory from patterns #164

Merged
merged 3 commits into from
Apr 6, 2024

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Apr 5, 2024

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 concrete Node types. I could maybe simplify them a little by using NodeWithSource, but I'm afraid it will be infeasible to make them abstract enough to use impl AstNode later on.

So instead, I propose to split the factory functions from the patterns and create a pattern_factory module for them, which is what this PR does. I've only created factory functions for Accessor and Step so far, and I've also moved the compiler (which directly uses TreeSitter of course) into the pattern_factory directory. I've moved auto_wrap.rs too, since it's only used by compiler.rs and step.rs.

This creates a bit of a circular dependency between the pattern module and the pattern_factory module, but that should be temporary. If we agree on this approach, I'll continue moving the functions out of the other patterns until pattern doesn't depend on pattern_factory anymore. And of course, the end goal is still for pattern to not depend on TreeSitter at all anymore, so eventually it can be moved wholesale into the future grit-core-patterns crate.

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

    • Introduced a new module pattern_factory to enhance pattern handling capabilities.
    • Added constants for index values and default file names to improve standardization across patterns.
  • Refactor

    • Streamlined import statements across various pattern-related files for improved clarity and maintainability.
    • Restructured the Accessor struct and renamed Key to AccessorKey for better code readability.
    • Consolidated imports under super and pattern_factory to maintain consistency in the codebase.
    • Simplified and refactored the implementation of the Step struct, removing unnecessary pattern checks.
  • Style

    • Reorganized imports and adjusted module structures across numerous files to align with coding standards and enhance code organization.

Copy link
Contributor

coderabbitai bot commented Apr 5, 2024

Walkthrough

The 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 pattern_compiler and pattern_factory, consolidation of imports related to CompilationContext, and streamlining of import paths for improved code organization.

Changes

Files Change Summary
.../src/analyze.rs, .../commands/apply_pattern.rs, .../src/result_formatting.rs Adjusted imports for Position, CompilationResult, and pattern-related entities; moved DEFAULT_FILE_NAME import.
.../core/src/lib.rs Added pattern_factory module to public interface.
.../core/src/pattern/{accessor,accumulate,add,...,variable,where}.rs Reorganized imports, particularly CompilationContext and Context; introduced new modules like constants, paths, and runtime; simplified code structures.
.../core/src/pattern/constants.rs Added constants for index values and default file name.

"In the realm of code, where logic intertwines,
A rabbit hopped through, refining the lines.
Imports aligned, with precision so fine,
A factory of patterns, in harmony, they align.
🐇💻✨"

"With each hop and skip, the codebase did glow,
Organized and streamlined, the changes did show.
A celebration of order, in binary prose,
The rabbit's work done, on to new adventures, it goes."

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

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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration 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/coderabbit-overrides.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.

@morgante
Copy link
Contributor

morgante commented Apr 5, 2024

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:

  • I prefer to not call things "factories" (since that is generic). Can we call these individual compilers? Ex. AccessorCompiler.
  • Instead of free-floating functions, there should probably be a struct for each implementing a NodeCompiler trait, so from_node always has the same signature.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Review Status

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between e311c2a and 60e251d.
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 the pattern_factory module to explain its purpose and usage.

crates/core/src/pattern/predicate_return.rs (1)

8-8: Ensure that moving CompilationContext to pattern_factory does not introduce circular dependencies.

crates/core/src/pattern/equal.rs (1)

7-7: Ensure that moving CompilationContext to pattern_factory does not introduce circular dependencies.

crates/core/src/pattern/add.rs (1)

7-7: Ensure that moving CompilationContext to pattern_factory does not introduce circular dependencies.

crates/core/src/pattern/divide.rs (1)

7-7: Ensure that moving CompilationContext to pattern_factory does not introduce circular dependencies.

crates/core/src/pattern/multiply.rs (1)

7-7: Ensure that moving CompilationContext to pattern_factory does not introduce circular dependencies.

crates/core/src/pattern/subtract.rs (1)

7-7: Ensure that moving CompilationContext to pattern_factory does not introduce circular dependencies.

crates/core/src/pattern/modulo.rs (1)

7-7: Ensure that moving CompilationContext to pattern_factory does not introduce circular dependencies.

crates/core/src/pattern/like.rs (1)

8-8: The reorganization of imports, specifically moving CompilationContext under pattern_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 in map.rs, particularly moving CompilationContext to pattern_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 in limit.rs, moving CompilationContext to pattern_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 in predicate_definition.rs, particularly adjusting the paths for Context and CompilationContext, 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 in sequential.rs, including the use of step_from_node from pattern_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 in after.rs, moving CompilationContext and debug to pattern_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 in before.rs, adjusting the import statements for CompilationContext, 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 in maybe.rs, moving CompilationContext to pattern_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 use accessor_from_node for creating Accessor instances 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: The new method for Accessor is straightforward and correctly implemented.


39-40: Ensure proper error handling for the Variable variant in get_key when the variable's text cannot be retrieved.

Verification successful

The Variable::text(state) method implementation in variable.rs has been located. This method returns a Result<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's Result type 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 include Pattern from the patterns module is appropriate for the file's context.


8-8: The addition of CompilationContext from pattern_factory is 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 for CompilationContext aligns with the refactor's goal of better organizing the codebase.

crates/core/src/pattern/dynamic_snippet.rs (3)

4-4: The usage of constants DEFAULT_FILE_NAME and GLOBAL_VARS_SCOPE_INDEX is clear and well-defined.


9-9: The reorganization of imports to include Variable from the variable module is appropriate and improves the file's clarity.


12-12: The addition of split_snippet from the crate level 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 for CompilationContext aligns 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 for CompilationContext aligns 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 for CompilationContext aligns 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 include CompilationContext from the pattern_factory module 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 of wrap_pattern_in_before_and_after_each_file from the from_node method does not negatively impact the pattern handling logic.

Verification successful

The removal of wrap_pattern_in_before_and_after_each_file from the from_node method 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 moving CompilationContext, 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 for Context and CompilationContext support 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 for CompilationContext and Context, enhances the modularity of the codebase.

crates/core/src/pattern/accumulate.rs (1)

1-15: The reorganization of imports under super and 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, including binding::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 the MATCH_VAR import enhance clarity and maintainability.


7-7: Moving Effect and FileOwner declarations 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 of CompilationContext within the pattern_factory::compiler module.

crates/core/src/pattern/rewrite.rs (1)

11-11: The change in the import statement correctly reflects the new location of CompilationContext within the pattern_factory::compiler module.

crates/cli/src/analyze.rs (1)

22-25: The changes in the import statements correctly reflect the adjustments in the codebase for Position and CompilationResult.

crates/core/src/pattern/variable.rs (1)

2-2: The change in the import statement correctly reflects the new location of GLOBAL_VARS_SCOPE_INDEX within the constants module.

crates/cli/src/result_formatting.rs (1)

11-11: The change in the import statement correctly reflects the new location of DEFAULT_FILE_NAME within the constants module.

crates/core/src/pattern/call.rs (1)

13-13: Ensure that CompilationContext is 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: > 📝 NOTE

This 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 modules constants, paths, and runtime to 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 within patterns.rs to 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: > 📝 NOTE

This 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 Matcher and Name traits to explain their purpose and usage within the pattern matching system. This will improve code maintainability and readability for future contributors.


65-70: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [67-92]

Ensure that the Pattern enum 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 the accessor_from_node function correctly handles all edge cases and errors. Since it's a critical part of creating Accessor patterns, thorough testing and error handling are essential to ensure robustness.

Comment on lines +15 to 18
pub enum AccessorMap {
Container(Container),
Map(GritMap),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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),
}

Comment on lines +22 to +23
pub map: AccessorMap,
pub key: AccessorKey,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
pub map: AccessorMap,
pub key: AccessorKey,
pub map: AccessorMap,
pub key: Key,

Comment on lines +27 to 30
pub enum AccessorKey {
String(String),
Variable(Variable),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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),
}

Comment on lines +33 to 36
pub fn new(map: AccessorMap, key: AccessorKey) -> Self {
Self { map, key }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Status

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 60e251d and 717c3d3.
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 in mod.rs are well-organized and follow Rust best practices.

crates/core/src/pattern_compiler/node_compiler.rs (1)

1-20: The NodeCompiler trait 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 in search.rs align with the PR's objective of reorganizing the codebase for better modularity and clarity.


2-2: The logic within the search_query function 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 the NodeCompiler trait for AccessorCompiler is 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 in within.rs align with the PR's objective of reorganizing the codebase for better modularity and clarity.


7-7: The implementations of the Name and Matcher traits for Within are 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 in apply.rs align 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 in check.rs align 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 the NodeCompiler trait for StepCompiler is 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 of auto_wrap_pattern to pub(super) aligns with the intended encapsulation and usage within the module hierarchy.


1-16: > 📝 NOTE

This 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, and StringConstant are 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.

@morgante
Copy link
Contributor

morgante commented Apr 6, 2024

@arendjr Please request review when this is ready (or mark as draft).

@arendjr
Copy link
Contributor Author

arendjr commented Apr 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants