-
Notifications
You must be signed in to change notification settings - Fork 125
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
[WIP] KCL watch system #1212
[WIP] KCL watch system #1212
Conversation
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. |
@@ -0,0 +1,5 @@ | |||
{ |
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.
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.
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.
okay, I thought for better readability i tend to make a new file for it.
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.
No, this file should be optional, otherwise it will increase the cost of release.
kclvm/tools/src/LSP/src/main_loop.rs
Outdated
@@ -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 |
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.
Should it serve on PWD or in the user's workspace?
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.
currently for PWD
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.
Why for pwd. In pwd how we watch user kcl files.
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.
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.
@He1pa Can you help review and provide some more professional comments and guidance? Thank you! |
Cloud you please use |
okay |
Hello @octonawish-akcodes Kind reminder, LXF 1 Term is one week away, are you still working on this PR? |
Yes Ill raise an update today |
99b8140
to
0da9aae
Compare
0effa65
to
f34c2b4
Compare
Signed-off-by: Abhishek Kumar <[email protected]>
PR conflict |
} | ||
|
||
/// Get the configuration file path | ||
pub fn get_config_file() -> Option<PathBuf> { |
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.
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.
pub fn new(file: &DirEntry) -> Self { | ||
let metadata = file.metadata().unwrap(); | ||
File { | ||
name: file.file_name().to_str().unwrap().to_string(), |
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.
Avoiding the unwrap()
function calling.
|
||
/// Iterator for file events | ||
pub fn iter_events(&mut self) -> impl Iterator<Item = FileEvent> + '_ { | ||
let interval = Duration::from_millis(500); |
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.
Why use 500
here?
false => WalkDir::new(config.path()).min_depth(1).max_depth(1), | ||
} | ||
.into_iter() | ||
.filter(|x| x.as_ref().unwrap().metadata().unwrap().is_file()) |
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.
Avoiding the unwrap()
function calling. Need to carefully check other places where the unwrap()
function is called.
} | ||
|
||
/// KCL Watch System structure to manage the observer and handler registry | ||
pub struct KCLWatchSystem { |
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.
pub struct KCLWatchSystem { | |
pub struct WatchSystem { |
@@ -28,6 +30,9 @@ mod formatting; | |||
#[cfg(test)] | |||
mod tests; | |||
|
|||
mod file; | |||
mod kcl_watch_system; // Import the new module |
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.
mod kcl_watch_system; // Import the new module | |
mod watch_system; // Import the new module |
let config = | ||
config_manager::Config::load_from_file(&config_file).expect("Failed to load configuration"); | ||
|
||
/// Create a new KCL Watch System instance |
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.
Use the normal comment //
let kcl_watch_system = KCLWatchSystem::new(config.clone()); | ||
|
||
// Start the observer | ||
kcl_watch_system.start_observer(); |
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.
Do not start the watch system in the language server. Just use the watcher in the kcl language server.
Close due to inactive state |
KCL watch system according to the design specified, work is under process
Changes Made:
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.
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.
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.
Defined the FileHandler trait and implemented the handle method for handling file events.
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.