-
Notifications
You must be signed in to change notification settings - Fork 194
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
Load rope_autoimport
cache on workspace/didChangeConfiguration
#461
Load rope_autoimport
cache on workspace/didChangeConfiguration
#461
Conversation
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.
Small suggestion for you @tkrabel-db, the rest looks good to me.
Co-authored-by: Carlos Cordoba <[email protected]>
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.
Thanks @tkrabel-db!
""" | ||
Initialize autoimport if it has been enabled through a | ||
workspace/didChangeConfiguration message from the frontend. | ||
|
||
Generates the cache for local and global items. | ||
""" | ||
if config.plugin_settings("rope_autoimport").get("enabled", False): | ||
_reload_cache(config, 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.
It could keep the last enabled state and only reload cache on setting actually changing. Otherwise it can trigger wasteful reloads on any other setting changing.
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.
Yeah, that's a good very idea, thanks @rchl!
@tkrabel-db, what do you think? We could use a global variable to track the plugin's enabled state and only reload the cache when changing from ENABLED = False
to True
.
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.
Will do that!
Fixes #460
How is this tested?
Manually, ensuring that autoimports works after sending a
workspace/didChangeConfiguration
to the backend.