-
Notifications
You must be signed in to change notification settings - Fork 0
Staging #8
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
Staging #8
Conversation
…able this for later.
|
|
✋ Hey $contributorName, 🛑 thanks for your contribution! Before we can accept it, we need you to sign our contributors license agreement (CLA). 🖊️ I have read the Thread contributors license agreement and I agree to it. Adam Poulemanos seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
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.
Pull Request Overview
This pull request introduces a complete new thread-rule-engine crate, which is a fork of the ast-grep-config crate with significant modifications for thread-specific requirements. The PR adds comprehensive rule engine functionality including parsing, validation, transformation, and execution of code analysis rules, along with extensive analysis tools for serialization dependency management.
Key changes include:
- Complete rule engine implementation with YAML-based configuration support
- Serialization dependency analysis tools and separation strategy documentation
- Comprehensive benchmark suite for performance testing
- Feature-gated architecture preparation for future WASM deployment
Reviewed Changes
Copilot reviewed 105 out of 191 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rule-engine/Cargo.toml | Main crate configuration with dependencies and feature flags |
| crates/rule-engine/src/lib.rs | Core library exports and YAML parsing functionality |
| crates/rule-engine/src/rule/deserialize_env.rs | Rule deserialization environment and dependency management |
| crates/rule-engine/src/maybe.rs | Optional field serialization wrapper with custom serde logic |
| crates/rule-engine/src/label.rs | Diagnostic label system for code analysis results |
| crates/rule-engine/src/fixer.rs | Code fixing and transformation system |
| crates/rule-engine/src/combined.rs | Multi-rule scanning with suppression support |
| crates/rule-engine/src/check_var.rs | Variable validation and scope checking |
| crates/rule-engine/serialization_analysis/ | Comprehensive tooling for analyzing serialization dependencies |
| crates/rule-engine/benches/ | Performance benchmark suite |
Comments suppressed due to low confidence (2)
crates/rule-engine/src/rule/deserialize_env.rs:41
- [nitpick] The type alias
OrderResultis ambiguous and doesn't clearly indicate it's for dependency ordering. Consider renaming toDependencyOrderResultorTopologicalSortResult.
type OrderResult<T> = Result<T, String>;
crates/rule-engine/benches/simple_benchmarks.rs:49
- The benchmark references a test data file that doesn't exist in the diff. Ensure the test data files are included or the benchmark will fail.
test_code: include_str!("../test_data/sample_typescript.ts"),
| } | ||
| } | ||
|
|
||
| const ERROR_STR: &str = r#"Maybe fields need to be annotated with: |
Copilot
AI
Jul 19, 2025
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.
[nitpick] The hardcoded error message spans multiple lines and could be improved for clarity. Consider using a more descriptive constant name like SERDE_ANNOTATION_ERROR_MSG.
| const ERROR_STR: &str = r#"Maybe fields need to be annotated with: | |
| const SERDE_ANNOTATION_ERROR_MSG: &str = r#"Maybe fields need to be annotated with: |
| const IGNORE_TEXT: &str = "ast-grep-ignore"; | ||
|
|
Copilot
AI
Jul 19, 2025
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.
The hardcoded ignore text 'ast-grep-ignore' should be configurable or at least documented as a magic string. Consider making it a configurable constant.
| const IGNORE_TEXT: &str = "ast-grep-ignore"; | |
| pub struct Config { | |
| pub ignore_text: String, | |
| } | |
| impl Default for Config { | |
| fn default() -> Self { | |
| Self { | |
| ignore_text: "ast-grep-ignore".to_string(), | |
| } | |
| } | |
| } |
|
|
||
| for (idx, rule) in rules.iter().enumerate() { | ||
| let Some(kinds) = rule.matcher.potential_kinds() else { | ||
| eprintln!("rule `{}` must have kind", &rule.id); |
Copilot
AI
Jul 19, 2025
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.
Using eprintln! directly in library code is not ideal. Consider using a proper logging framework or returning an error instead of printing to stderr.
| pub(crate) fn used_vars(&self) -> RapidSet<&str> { | ||
| self.template.used_vars() |
Copilot
AI
Jul 19, 2025
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.
[nitpick] The method returns a set of string references but doesn't specify the lifetime relationship clearly. Consider using explicit lifetime parameters or returning owned strings for better API clarity.
| pub(crate) fn used_vars(&self) -> RapidSet<&str> { | |
| self.template.used_vars() | |
| pub(crate) fn used_vars(&self) -> RapidSet<String> { | |
| self.template.used_vars().into_iter().map(|s| s.to_string()).collect() |
No description provided.