fix: simplify the lazy approach #483
Conversation
WalkthroughWalkthroughThe changes involve significant modifications across multiple files, including the removal of the Changes
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
Additional context usedLearnings (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
Outside diff range, codebase verification and nitpick comments (3)
crates/grit-pattern-matcher/src/pattern/dynamic_snippet.rs (1)
54-57: LGTM! Consider adding tests for the new method.The code changes are approved. The new
from_str_constantmethod provides a straightforward way to create a constantDynamicPatternfrom a string input, which enhances the usability of theDynamicPattern.To ensure the correctness and maintainability of the code, consider adding tests for the new
from_str_constantmethod. For example:#[test] fn test_from_str_constant() { let pattern = DynamicPattern::from_str_constant("hello").unwrap(); assert!(matches!(pattern, DynamicPattern::Snippet(DynamicSnippet { parts }) if parts.len() == 1)); if let DynamicPattern::Snippet(DynamicSnippet { parts }) = pattern { assert!(matches!(parts[0], DynamicSnippetPart::String(s) if s == "hello")); } }crates/core/src/pattern_compiler/builder.rs (1)
244-248: LGTM! Consider adding documentation for the new methods.The code changes are approved. However, it would be beneficial to add documentation for the new
wrap_with_rewriteandwrap_with_accumulatemethods to explain their purpose and usage.+/// Wrap the pattern with a rewrite. +/// This allows for the substitution of patterns with a specified replacement. pub fn wrap_with_rewrite(self, replacement: DynamicPattern<MarzanoQueryContext>) -> Self { let pattern = Pattern::Rewrite(Box::new(Rewrite::new(self.pattern, replacement, None))); Self { pattern, ..self } } +/// Wrap the pattern with an accumulate. +/// This facilitates the accumulation of patterns, enabling more complex pattern compositions. pub fn wrap_with_accumulate(self, other: Pattern<MarzanoQueryContext>) -> Self { let pattern = Pattern::Accumulate(Box::new(Accumulate::new(self.pattern, other, None))); Self { pattern, ..self } }crates/core/src/marzano_resolved_pattern.rs (1)
53-65: LGTM! But suppress the clippy warning.The code changes are approved. The new
matchesmethod looks good and the use of the memory leak technique is justified for short-lived programs.However, the clippy static analysis tool has flagged this method as unused, which is a false positive in this case. The
pub(crate)visibility modifier indicates that the method is intended to be used within the same crate, even if it's not used yet.To suppress the warning, add the
#[allow(dead_code)]attribute above the method:+#[allow(dead_code)] pub(crate) fn matches( &self, pattern: &Pattern<MarzanoQueryContext>, state: &mut State<'a, MarzanoQueryContext>, context: &'a MarzanoContext<'a>, logs: &mut AnalysisLogs, ) -> GritResult<bool> {Tools
GitHub Check: clippy
[failure] 53-53: method
matchesis never used
error: methodmatchesis never used
--> crates/core/src/marzano_resolved_pattern.rs:53:19
|
41 | impl<'a> MarzanoResolvedPattern<'a> {
| ----------------------------------- method in this implementation
...
53 | pub(crate) fn matches(
| ^^^^^^^
|
= note:-D dead-codeimplied by-D warnings
= help: to override-D warningsadd#[allow(dead_code)]
We don't really need a new struct, and leaking memory is safer than transmuting.
Summary by CodeRabbit
New Features
matchesmethod inMarzanoResolvedPatternfor enhanced pattern matching capabilities.wrap_with_rewriteandwrap_with_accumulatemethods toPatternBuilderfor improved pattern manipulation.from_str_constantinDynamicPatternallows creation of patterns from string literals.Bug Fixes
ResolvedPatternfor better readability.Refactor
LazyTraversalstruct, simplifying the callback function signatures.Style