[WIP] KCL watch system #1212

octonawish-akcodes · 2024-04-10T04:26:09Z

KCL watch system according to the design specified, work is under process

Changes Made:

Handler Registry (kcl_watch_system.rs):
Implemented the HandlerRegistry structure to register and manage file handlers.
Added methods to register and get handlers for specific file types.
Implemented the handle_event method to handle file modification events by invoking the corresponding handler.
KCL Watch System (kcl_watch_system.rs):
Implemented the KCLWatchSystem structure to manage the observer and handler registry.
Added a method to start the observer and handle file events using the handler registry.
Observer (file.rs):
Implemented the Observer structure to watch files based on the provided configuration.
Added an iterator method (iter_events) to iterate over file events and filter them based on file modification times.
File Handler (file.rs):
Defined the FileHandler trait and implemented the handle method for handling file events.
Configuration Manager (config_manager.rs):
Implemented the Config structure to load configuration settings from a JSON file.
Added methods to retrieve the path, recursive flag, and file patterns from the configuration.

octonawish-akcodes · 2024-04-10T04:32:24Z

Working on the event handling and testing of this system, I have used JSON configuration instead of TOML cuz I found it user-friendly and easy to use, notify lib isn't used because of the deprecation of the file modification events there.

Peefy · 2024-04-10T08:14:23Z

kclvm/tools/src/LSP/src/observer.json

@@ -0,0 +1,5 @@
+{


I think this configuration file is unnecessary because it is only used for internal developer extension functions and not for KCL users, so it can be written in the code.

okay, I thought for better readability i tend to make a new file for it.

No, this file should be optional, otherwise it will increase the cost of release.

kclvm/tools/src/LSP/src/config_manager.rs

Peefy · 2024-04-10T13:25:42Z

kclvm/tools/src/LSP/src/main_loop.rs

@@ -11,6 +13,9 @@ pub(crate) fn main_loop(
    config: Config,
    initialize_params: InitializeParams,
 ) -> anyhow::Result<()> {
+    let kcl_watch_system = KCLWatchSystem::new(conf.clone());
+    kcl_watch_system.start_observer(); // Start the observer here


Should it serve on PWD or in the user's workspace?

currently for PWD

Why for pwd. In pwd how we watch user kcl files.

files will be watched based on the path user will defined in the config file, and the watcher will watch the files based on the file extension and trigger the corresponding event handler.

kclvm/tools/src/LSP/src/main.rs

kclvm/tools/src/LSP/src/kcl_watch_system.rs

Peefy · 2024-04-10T13:29:57Z

@He1pa Can you help review and provide some more professional comments and guidance? Thank you!

Peefy · 2024-04-11T02:50:22Z

Cloud you please use /// doc comments instead of // normal comments.

octonawish-akcodes · 2024-04-11T02:56:51Z

Cloud you please use /// doc comments instead of // normal comments.

okay

Peefy · 2024-05-16T07:20:21Z

Hello @octonawish-akcodes

Kind reminder, LXF 1 Term is one week away, are you still working on this PR?

octonawish-akcodes · 2024-05-16T07:28:02Z

Yes Ill raise an update today

kclvm/tools/src/LSP/src/kcl_watch_system.rs

Signed-off-by: Abhishek Kumar <abhishek22512@gmail.com>

Peefy · 2024-05-24T04:33:22Z

PR conflict

Peefy · 2024-05-24T05:06:02Z

kclvm/tools/src/LSP/src/config_manager.rs

+}
+
+/// Get the configuration file path
+pub fn get_config_file() -> Option<PathBuf> {


Avoid reading from this configuration file, we only need to maintain scalability at the code level, without the need to open configuration content to users.

Peefy · 2024-05-24T05:07:07Z

kclvm/tools/src/LSP/src/file.rs

+    pub fn new(file: &DirEntry) -> Self {
+        let metadata = file.metadata().unwrap();
+        File {
+            name: file.file_name().to_str().unwrap().to_string(),


Avoiding the unwrap() function calling.

Peefy · 2024-05-24T05:07:35Z

kclvm/tools/src/LSP/src/file.rs

+
+    /// Iterator for file events
+    pub fn iter_events(&mut self) -> impl Iterator<Item = FileEvent> + '_ {
+        let interval = Duration::from_millis(500);


Why use 500 here?

Peefy · 2024-05-24T05:08:20Z

kclvm/tools/src/LSP/src/file.rs

+        false => WalkDir::new(config.path()).min_depth(1).max_depth(1),
+    }
+    .into_iter()
+    .filter(|x| x.as_ref().unwrap().metadata().unwrap().is_file())


Avoiding the unwrap() function calling. Need to carefully check other places where the unwrap() function is called.

Peefy · 2024-05-24T05:09:02Z

kclvm/tools/src/LSP/src/kcl_watch_system.rs

+}
+
+/// KCL Watch System structure to manage the observer and handler registry
+pub struct KCLWatchSystem {


Suggested change

pub struct KCLWatchSystem {

pub struct WatchSystem {

Peefy · 2024-05-24T05:09:27Z

kclvm/tools/src/LSP/src/main.rs

@@ -28,6 +30,9 @@ mod formatting;
 #[cfg(test)]
 mod tests;

+mod file;
+mod kcl_watch_system; // Import the new module


Suggested change

mod kcl_watch_system; // Import the new module

mod watch_system; // Import the new module

Peefy · 2024-05-24T05:09:48Z

kclvm/tools/src/LSP/src/main.rs

+    let config =
+        config_manager::Config::load_from_file(&config_file).expect("Failed to load configuration");
+
+    /// Create a new KCL Watch System instance


Use the normal comment //

Peefy · 2024-05-24T05:10:25Z

kclvm/tools/src/LSP/src/main.rs

+    let kcl_watch_system = KCLWatchSystem::new(config.clone());
+
+    // Start the observer
+    kcl_watch_system.start_observer();


Do not start the watch system in the language server. Just use the watcher in the kcl language server.

octonawish-akcodes marked this pull request as draft April 10, 2024 04:26

Peefy reviewed Apr 10, 2024

View reviewed changes

Peefy assigned octonawish-akcodes May 7, 2024

octonawish-akcodes force-pushed the ws branch 2 times, most recently from 99b8140 to 0da9aae Compare May 22, 2024 10:10

Peefy reviewed May 22, 2024

View reviewed changes

kclvm/tools/src/LSP/src/kcl_watch_system.rs Outdated Show resolved Hide resolved

octonawish-akcodes force-pushed the ws branch 2 times, most recently from 0effa65 to f34c2b4 Compare May 22, 2024 11:50

KCL watch system

ccf069e

Signed-off-by: Abhishek Kumar <abhishek22512@gmail.com>

octonawish-akcodes force-pushed the ws branch from e0577c3 to ccf069e Compare May 23, 2024 08:57

Peefy reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] KCL watch system #1212

[WIP] KCL watch system #1212

octonawish-akcodes commented Apr 10, 2024

octonawish-akcodes commented Apr 10, 2024

Peefy Apr 10, 2024

octonawish-akcodes Apr 11, 2024

Peefy Apr 11, 2024

Peefy Apr 10, 2024

octonawish-akcodes Apr 11, 2024

Peefy Apr 11, 2024

octonawish-akcodes May 23, 2024

Peefy commented Apr 10, 2024

Peefy commented Apr 11, 2024

octonawish-akcodes commented Apr 11, 2024

Peefy commented May 16, 2024

octonawish-akcodes commented May 16, 2024

Peefy commented May 24, 2024

Peefy May 24, 2024

Peefy May 24, 2024

Peefy May 24, 2024

Peefy May 24, 2024

Peefy May 24, 2024

Peefy May 24, 2024

Peefy May 24, 2024

Peefy May 24, 2024

	mod kcl_watch_system; // Import the new module
	mod watch_system; // Import the new module

[WIP] KCL watch system #1212

Are you sure you want to change the base?

[WIP] KCL watch system #1212

Conversation

octonawish-akcodes commented Apr 10, 2024

octonawish-akcodes commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peefy commented Apr 10, 2024

Peefy commented Apr 11, 2024

octonawish-akcodes commented Apr 11, 2024

Peefy commented May 16, 2024

octonawish-akcodes commented May 16, 2024

Peefy commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment