-
Notifications
You must be signed in to change notification settings - Fork 23
[IMP] server,vscode: new config structure #315
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
Conversation
|
|
4 similar comments
|
|
|
|
|
|
|
|
|
|
1 similar comment
|
|
|
|
|
|
4 similar comments
|
|
|
|
|
|
|
|
|
|
3 similar comments
|
|
|
|
|
|
|
|
7c13674 to
d6e3688
Compare
|
|
|
|
server/src/cli_backend.rs
Outdated
| config.additional_stubs = self.cli.stubs.clone().unwrap_or(vec![]); | ||
| config.stdlib = self.cli.stdlib.clone().unwrap_or(S!("")); | ||
| config.additional_stubs = self.cli.stubs.clone().unwrap_or(vec![]).into_iter().collect(); | ||
| // config.stdlib = self.cli.stdlib.clone().unwrap_or(S!("")); |
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.
Did you remove no_typeshed and stdlib temporarily?
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 did that a while ago, I think it is a mistake. I might have removed no_typeshed because it was in the Config Struct, but never actually set, and it was always false.
I do now see that I made a mistake because it was in the CLI Args, is it intentional to keep it only settable from the CLI?
Or was it a missing implementation that it was not set in the config too ?
So I should integrate it into the new config, right?
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.
Also, Config.stdlib_dir was always set to an empty str.
So this code was never reached. and the same with no_typeshed
However, the right thing to do is not to remove them
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.
So I should add stdlib and no_typeshed to the new config, given that they were not in the old one?
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.
Yes, they are actually parameters for CLI only (used on runbot for example). std_lib is maybe never used for now, but I wanted to make it parametrizable.
Anyway we will need to add more parameters in the future
server/src/core/file_mgr.rs
Outdated
| pub struct FileMgr { | ||
| pub files: HashMap<String, Rc<RefCell<FileInfo>>>, | ||
| workspace_folder: Vec<String>, | ||
| workspace_folder: HashMap<String, 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.
what happen if you open two directories with the same name? Isn't it colliding?
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 will test that, I suppose the client should somehow distinguish them in some way.
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 am wrong, the name is not unique, I will have to fix that. Will push a commit to handle that
| serde_json::Value::String(config_file.to_html_string()) | ||
| ); | ||
| session.send_notification( | ||
| "$Odoo/setConfiguration", |
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.
New custom messages should be documented here: https://github.com/odoo/odoo-ls/wiki/Implement-a-custom-tool-with-the-server
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.
Updated
| }; | ||
| let selected_config = match maybe_selected_config { | ||
| None => { | ||
| session.show_message(MessageType::INFO, String::from("No Odoo configuration selected. Please select a configuration in the settings.")); |
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.
Shouldn't we take the first by default if the setting doesn't exist in the config?
Else any IDE implementation that doesn't implement the config switch interface won't be able to use it?
I don't know
|
|
|
|
No description provided.