-
Notifications
You must be signed in to change notification settings - Fork 11
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
Telescope support #6
Conversation
- removed dependency to config from api - refactored config to only handle config -> json stuff is done in json module - adapted config class to allow user to specify adapter - removed unneccessary call to config.setup in plugin/cd-project.lua
Hi stefanwatt, Really thanks for your efforts and this PR! I think split into those modules are nice and easier for later enhancements and maintainance. I have some ideas wants to discuss:How to use the config module.I see you pass it as a variable into functions, which is good to make it explicit. So, like the Api functions. I want to make the functions easier for user to call anywhere else. They don't need to consider about what config args they need to prepare and pass it into the API. |
@@ -8,12 +8,3 @@ if vim.g.loaded_cd_project == 1 then | |||
return | |||
end | |||
vim.g.loaded_cd_project = 1 | |||
|
|||
require("cd-project").setup() |
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.
If we remove this setup call in the plugin entrypoint, the user have to call the setup function when they add the plugin in their configuration.
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.
Isn't that desirable?
Currently it's running twice if you have a custom configuration.
Yeah to be honest I also wasn't sure how to do it. Do you have a pattern in mind? |
I don't realise that I was calling something from API in CONFIG? So in my mind, the layer should be something like
So API can directly invoke anything exposed in config |
I'll check again when I get off work, but there definitely was a cyclical dependency. Maybe it wasn't there before the PR and was a result of the refactoring. What do you think about using a global variable to make the config accessible? I just think it's kinda weird for other modules to import config. Seems like that should be something that's only used to call the setup function. |
Global variables are also a quite good option for configuration, but I think community is more in to the setup function. I suppose that other modules require config is quite common? Since the behaviours are controlled by the configuration, it will always have this kind of dependency, no matters it is passed in by parameters or imported directly. |
I just pushed something. I couldn't find another clean way to require config everywhere. At some point you still end up with a cyclical dependency. For example: |
Sorry, just found that the merge is not based on your branch, my bad. |
fixes #1