crates/cli/src/config.rs (2)

219-220: Remove unnecessary empty line

Two consecutive empty lines are not needed here. Please remove one.

-
-
+

---

92-102: Fix formatting issues with cargo fmt

The pipeline indicates formatting issues in this code segment. Please run cargo fmt to fix the code style.

Additionally, per the previous review comment, consider whether this method should be moved to crates/cli/src/lsp.rs since it's specifically for LSP usage.

#!/bin/bash
# Verify if this method is only used by LSP
rg -A 2 "make_rule_finder" --type rust

crates/lsp/Cargo.toml (1)

29-30: Verify if ignore and serde_yaml dependencies are used

Previous review comments indicated these dependencies might not be used. Please verify if they are actually needed.

#!/bin/bash
# Check if 'ignore' crate is used in the lsp crate
echo "Checking usage of 'ignore' crate:"
rg "use ignore::" crates/lsp/src/
rg "ignore::" crates/lsp/src/

echo -e "\nChecking usage of 'serde_yaml' in the lsp crate:"
rg "use serde_yaml" crates/lsp/src/
rg "serde_yaml::" crates/lsp/src/

crates/lsp/src/lib.rs (5)

338-349: Reduce indentation by flattening nested conditions

The nested if-let can be flattened for better readability.

-      if let Ok(rules) = self.rules.read() {
-        if let Some(note) = rules.get_rule(id).and_then(|r| r.note.clone()) {
-          let start = diagnostic.range.start;
-          let end = diagnostic.range.end;
-          let atom = self
-            .interner
-            .entry(id.clone())
-            .or_insert_with(|| Arc::new(note.clone()))
-            .clone();
-          notes.insert((start.line, start.character, end.line, end.character), atom);
-        }
-      }
+      let Ok(rules) = self.rules.read() else { continue };
+      let Some(note) = rules.get_rule(id).and_then(|r| r.note.clone()) else { continue };
+      let start = diagnostic.range.start;
+      let end = diagnostic.range.end;
+      let atom = self
+        .interner
+        .entry(id.clone())
+        .or_insert_with(|| Arc::new(note.clone()))
+        .clone();
+      notes.insert((start.line, start.character, end.line, end.character), atom);

---

717-728: Reduce indentation in the loop

Flatten the nested if-let structure for better readability.

     for uri_str in open_files {
-      if let Ok(uri) = uri_str.parse::<Uri>() {
-        if let Some(mut versioned) = self.map.get_mut(&uri_str) {
-          // Republish diagnostics for this file
-          if let Some(diagnostics) = self.get_diagnostics(&uri, &versioned) {
-            versioned.notes = self.build_notes(&diagnostics);
-            self
-              .client
-              .publish_diagnostics(uri, diagnostics, Some(versioned.version))
-              .await;
-          }
-        }
-      }
+      let Ok(uri) = uri_str.parse::<Uri>() else { continue };
+      let Some(mut versioned) = self.map.get_mut(&uri_str) else { continue };
+      
+      // Republish diagnostics for this file
+      let Some(diagnostics) = self.get_diagnostics(&uri, &versioned) else { continue };
+      versioned.notes = self.build_notes(&diagnostics);
+      self
+        .client
+        .publish_diagnostics(uri, diagnostics, Some(versioned.version))
+        .await;
     }

---

164-177: Remove redundant file type checking

The file watcher registration already specifies which files to watch (*.yml patterns). When this handler is called, it means a relevant file has changed, so just reload without additional filtering.

-    // Check if any of the changed files are configuration files
-    let mut config_changed = false;
-    for change in &params.changes {
-      let uri = &change.uri;
-      if let Some(path) = uri.to_file_path() {
-        if let Some(filename) = path.file_name().and_then(|n| n.to_str()) {
-          if filename == "sgconfig.yml" || filename.ends_with(".yml") || filename.ends_with(".yaml") {
-            config_changed = true;
-            break;
-          }
-        }
-      }
-    }
-
-    if config_changed {
+    // File watcher already filters for relevant files, so just reload
+    {

---

307-313: Extract URI to path conversion logic

This URI to path conversion logic should be abstracted into a helper method for reusability.

+  fn uri_to_relative_path(&self, uri: &Uri) -> Option<PathBuf> {
+    let absolute_path = uri.to_file_path().ok()?;
+    let path = if let Ok(p) = absolute_path.strip_prefix(&self.base) {
+      p.to_path_buf()
+    } else {
+      absolute_path
+    };
+    Some(path)
+  }
+
   fn get_diagnostics(
     &self,
     uri: &Uri,
     versioned: &VersionedAst<StrDoc<L>>,
   ) -> Option<Vec<Diagnostic>> {
-    let absolute_path = uri.to_file_path()?;
-    let path = if let Ok(p) = absolute_path.strip_prefix(&self.base) {
-      p
-    } else {
-      &absolute_path
-    };
+    let path = self.uri_to_relative_path(uri)?;
     
     let rules = self.rules.read().ok()?;
-    let rule_refs = rules.for_path(path);
+    let rule_refs = rules.for_path(&path);

---

633-641: Fix file watcher patterns

1. The sgconfig.yml pattern is redundant since *.yml already includes it
2. Add *.yaml pattern to support both YAML file extensions

-    let config_watcher = FileSystemWatcher {
-      glob_pattern: GlobPattern::String("**/sgconfig.yml".to_string()),
-      kind: Some(WatchKind::Create | WatchKind::Change | WatchKind::Delete),
-    };
-    
-    let rule_watcher = FileSystemWatcher {
+    let yml_watcher = FileSystemWatcher {
       glob_pattern: GlobPattern::String("**/*.yml".to_string()),
       kind: Some(WatchKind::Create | WatchKind::Change | WatchKind::Delete),
     };
+    
+    let yaml_watcher = FileSystemWatcher {
+      glob_pattern: GlobPattern::String("**/*.yaml".to_string()),
+      kind: Some(WatchKind::Create | WatchKind::Change | WatchKind::Delete),
+    };

Also update the registration to include both watchers:

         serde_json::to_value(DidChangeWatchedFilesRegistrationOptions {
-          watchers: vec![config_watcher, rule_watcher],
+          watchers: vec![yml_watcher, yaml_watcher],
         }).map_err(|e| tower_lsp_server::jsonrpc::Error::invalid_params(e.to_string()))?

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

248-249: Remove unnecessary empty line

Two consecutive empty lines are not needed here. Please remove one to maintain consistent formatting.

-
-
+

crates/lsp/tests/basic.rs (1)

319-352: Good test coverage for configuration file changes

The test correctly verifies the handling of watched file changes. Consider making the sleep duration configurable or using a more deterministic approach if possible.

-  // Give some time for processing
-  tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;
+  // Give some time for async processing
+  // Consider using a more deterministic approach if the test becomes flaky
+  tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1500
File: crates/cli/src/config.rs:60-60
Timestamp: 2024-10-08T18:41:57.037Z
Learning: When `find_rules` returns `(RuleCollection<SgLang>, RuleStats)`, we should handle both elements and add logging of `RuleStats` in the callers, such as in `crates/cli/src/verify.rs` and `crates/cli/src/lsp.rs`.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1500
File: crates/cli/src/config.rs:60-60
Timestamp: 2024-10-05T19:26:12.834Z
Learning: When `find_rules` returns `(RuleCollection<SgLang>, RuleStats)`, we should handle both elements and add logging of `RuleStats` in the callers, such as in `crates/cli/src/verify.rs` and `crates/cli/src/lsp.rs`.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1500
File: crates/cli/src/config.rs:60-60
Timestamp: 2024-10-05T19:26:12.834Z
Learning: When `find_rules` returns `(RuleCollection<SgLang>, RuleStats)`, we should handle both elements and add logging of `RuleStats` in the callers, such as in `crates/cli/src/verify.rs` and `crates/cli/src/lsp.rs`.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1546
File: crates/cli/src/utils/rule_overwrite.rs:118-121
Timestamp: 2024-10-22T13:56:44.052Z
Learning: The `OverwriteResult` struct in `crates/cli/src/utils/rule_overwrite.rs` will have more override options added in the future, so it should be retained even if it currently only contains an `Option<Severity>`.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#843
File: crates/config/src/fixer.rs:167-181
Timestamp: 2024-10-08T18:41:57.037Z
Learning: The `expand_end` function in `crates/config/src/fixer.rs` should return `node.range().end` instead of `node.range().start` to correctly calculate the end of the range.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1560
File: crates/cli/src/scan.rs:405-407
Timestamp: 2024-10-28T03:46:27.850Z
Learning: `ProjectConfig::setup` returns a `Result<Result<ProjectConfig>>`, so wrapping the result in `Ok()` is unnecessary.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1546
File: crates/cli/src/utils/rule_overwrite.rs:76-91
Timestamp: 2024-10-22T14:04:15.734Z
Learning: In the `ast-grep-cli` application code, avoid unnecessary abstractions; functions don't need to accept iterators instead of `Vec` if not required.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1560
File: crates/cli/src/lib.rs:0-0
Timestamp: 2024-10-28T03:45:43.551Z
Learning: In the `sg` CLI application, the `-l` or `--lang` command argument depends on the configuration and project being set up, so the configuration cannot be parsed using clap's `App` struct at that point.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1497
File: crates/cli/src/utils/mod.rs:174-175
Timestamp: 2024-09-29T21:59:50.675Z
Learning: ast-grep requires Rust 1.65 or newer. It's acceptable to use syntax and features introduced in Rust 1.65 or newer in ast-grep code.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1958
File: crates/core/src/tree_sitter.rs:262-268
Timestamp: 2025-04-28T19:41:29.191Z
Learning: The `Language` trait in ast-grep already has a `Clone` bound, so traits that extend `Language` (like `LanguageExt`) inherit this bound and don't need to specify it again.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1958
File: crates/core/src/tree_sitter.rs:262-268
Timestamp: 2025-04-28T19:41:29.191Z
Learning: The `Language` trait in ast-grep already has a `Clone` bound (`trait Language: Clone + 'static`), so traits that extend `Language` (like `LanguageExt`) inherit this bound and don't need to specify it again.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1652
File: Cargo.toml:35-38
Timestamp: 2024-12-08T20:23:06.671Z
Learning: In the ast-grep project, `tree-sitter-native` is not updated because `tree-sitter-facade-sg` requires a special patch update.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1630
File: crates/core/src/node.rs:710-718
Timestamp: 2024-11-30T01:59:37.083Z
Learning: In the `ast-grep` project, when writing tests for position calculations in `crates/core/src/node.rs`, positions should be calculated based on character counts rather than character widths.

Learnt from: philiptaron
PR: ast-grep/ast-grep#0
File: :0-0
Timestamp: 2024-07-27T01:25:27.298Z
Learning: In the `ast-grep` repository, the existence of the `sg` binary after the build process is guaranteed, making checks for its existence before installing shell completions unnecessary.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1500
File: crates/cli/src/config.rs:60-60
Timestamp: 2024-10-08T18:41:57.037Z
Learning: In the `ast-grep` codebase, it's acceptable for some callers of `find_rules` to intentionally ignore the `RuleStats` returned in its tuple `(RuleCollection<SgLang>, RuleStats)` when the statistics are not needed.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1497
File: crates/cli/src/lsp.rs:2-2
Timestamp: 2024-10-08T18:41:57.037Z
Learning: `ErrorContext` is exported from `crates/cli/src/utils/mod.rs` using `pub use`.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1497
File: crates/cli/src/utils/worker.rs:69-77
Timestamp: 2024-10-08T18:41:57.037Z
Learning: In `crates/cli/src/utils/worker.rs`, implementing `From<T>` for `Items<T>` is considered over-abstraction and should be avoided.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1664
File: crates/core/src/node.rs:744-746
Timestamp: 2024-12-14T21:30:27.243Z
Learning: In the Rust file `crates/core/src/node.rs` of the ast-grep project, the methods `start_pos()` and `end_pos()` retrieve positions from tree-sitter. When updating terminology from `row` to `line`, references to `pos.row()` within these methods should not be modified, as they rely on tree-sitter's API.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1546
File: crates/cli/src/lsp.rs:27-27
Timestamp: 2024-10-22T13:56:16.926Z
Learning: RuleOverwrite implements `Default` via the `#[derive(Default)]` macro.