-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Add support for config imports #565
base: master
Are you sure you want to change the base?
Conversation
8d6fd3d
to
2d34116
Compare
This looks like a great addition! 🙏 I haven't had a chance to look at it in detail yet, but I'll see if I can find some time in the next few days. |
lua/rocks/config/internal.lua
Outdated
end | ||
end | ||
end | ||
-- Merge, giving preference to existing config before imports |
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.
question: if a use case is to have configs that aren't checked into scm, wouldn't it be better to prioritise the imports?
Although, it might be best just to treat conflicts as an error.
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, you're right. You'd want imports to have preference over the base config so that you could have local config override scm config if you wanted to do that. Since this is a valid use case, we may now want to raise an error. I'll change it to give preference to the imported config.
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.
The reason I'm not sure if we want to support overrides is because it increases the complexity: When updating plugins that exist in multiple configs, you have to keep track of which toml file(s) to write to.
There might be other subtle things I'm not considering here.
I'm not sure if the ability to override scm configs is strong enough of a use case, which is why I'd say YAGNI for now.
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 (correct me if I'm wrong) because both the config and the operations respect config preferences, you will always write back to the config that has higher preference if it exists in both configs. The config.internal module merges all the configurations in order and the operations.helpers module orders the config tables by preference. So, I think it shouldn't add complexity but I may be wrong so I'll leave that up to you to decide.
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.
That's probably correct. But I'm still not sure if it has any implications I'm not foreseeing, which is why I'd rather not support overrides until people ask 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.
come to think about it, we should probably add some tests for this feature.
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'll add tests later today.
lua/rocks/operations/helpers.lua
Outdated
table.insert(paths, file_path) | ||
|
||
-- Follow import paths | ||
-- TODO: Remove call to get_imports, this is needed because toml-edit doesn't |
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.
issue: I'm personally not a fan of todo comments, as they tend to get forgotten (the ones we do have here are quite old).
Let's move this to an issue (I guess in toml-edit).
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.
Issue here: nvim-neorocks/toml-edit.lua#37. I'll remove the TODO after it's resolved, and we can maybe update toml-edit in this PR also.
Btw @mrcjkb I opened this issue for toml_edit.lua: (the lua library wrapper and not the original toml-edit) nvim-neorocks/toml-edit.lua#37. I'll take a look at it later but my rust is rusty so it will take me some time. Might be a quick fix for someone who is very familiar since it looks like you'd just need to match on a new toml-edit value type in the parse function ( |
Addresses: #240
This PR aims to add support for config importing/inclusion for the purpose of modularizing your rocks.toml and/or supporting local configuration that is outside of version control.
@vhyrro Had mention making this into a separate module but I think configuration imports is a fairly standard/core feature, so I wanted to implement it right in rocks.nvim. It also seemed easier to deal with the complexities of allowing core rocks.nvim and other modules interact with configuration as if it is one table but ensuring that we write all configuration back to the same files that they came from after modification.
Essentially, we'd need to make a breaking change to the handler API to support a list of
MutRocksTomlRef
so that modifications could be isolated to each file. To avoid this, I implemented this using a metatable which contains all the configuration tables associated with each config file and then uses the__index
and__newindex
methods to get the correct table for modification. For the config module, the imports are just merged into the base configuration since that isn't being written back to the config files.Side Note: There seems to be an issue in toml-edit where toml lists don't properly convert to a lua table when you use the
parse
function. Only string key tables seem to work for theparse
function. I may open an issue in the repo unless there's something I'm missing here.