Skip to content
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(config): import option for splitting rocks toml configs #565

Merged
merged 8 commits into from
Oct 25, 2024

Conversation

simifalaye
Copy link
Contributor

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 the parse function. I may open an issue in the repo unless there's something I'm missing here.

@mrcjkb
Copy link
Member

mrcjkb commented Oct 20, 2024

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.

README.md Show resolved Hide resolved
lua/rocks/config/internal.lua Show resolved Hide resolved
end
end
end
-- Merge, giving preference to existing config before imports
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@mrcjkb mrcjkb Oct 20, 2024

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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/fs.lua Outdated Show resolved Hide resolved
lua/rocks/fs.lua Outdated Show resolved Hide resolved
lua/rocks/operations/helpers.lua Outdated Show resolved Hide resolved
lua/rocks/operations/helpers.lua Outdated Show resolved Hide resolved
lua/rocks/operations/helpers.lua Outdated Show resolved Hide resolved
lua/rocks/operations/helpers.lua Outdated Show resolved Hide resolved
lua/rocks/operations/helpers.lua Outdated Show resolved Hide resolved
@simifalaye
Copy link
Contributor Author

simifalaye commented Oct 21, 2024

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 (toml_edit::Value::Array)

Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is failing on stylua.

@simifalaye
Copy link
Contributor Author

@mrcjkb Updated the flake.lock also but not sure if that's needed now for the CI? Feel free to take over the PR to get it through as I'm just new to nix. I believe all of the code, unit tests and style stuff should be addressed now but the CI might need some other things to get new toml-edit changes.

mrcjkb
mrcjkb previously approved these changes Oct 25, 2024
README.md Show resolved Hide resolved
@mrcjkb mrcjkb changed the title feat: Add support for config imports feat(rocks.toml): include option for importing rocks toml configs Oct 25, 2024
@mrcjkb mrcjkb changed the title feat(rocks.toml): include option for importing rocks toml configs feat(rocks.toml): import option for importing rocks toml configs Oct 25, 2024
@mrcjkb mrcjkb changed the title feat(rocks.toml): import option for importing rocks toml configs feat(rocks.toml): import option for splitting rocks toml configs Oct 25, 2024
@mrcjkb mrcjkb changed the title feat(rocks.toml): import option for splitting rocks toml configs feat(config): import option for splitting rocks toml configs Oct 25, 2024
@mrcjkb
Copy link
Member

mrcjkb commented Oct 25, 2024

Merging into the split-rocks-toml branch, so I can fix the build there.

@mrcjkb mrcjkb changed the base branch from master to split-rocks-toml October 25, 2024 14:04
@mrcjkb mrcjkb merged commit c90f451 into nvim-neorocks:split-rocks-toml Oct 25, 2024
4 of 7 checks passed
mrcjkb added a commit that referenced this pull request Oct 25, 2024
…#573)


Co-authored-by: Simi Falaye <simifalaye1@gmail.com>
@mrcjkb
Copy link
Member

mrcjkb commented Oct 31, 2024

@simifalaye I just noticed that since this PR, our integration test on Ubuntu is failing with

E5113: Error while calling lua chunk: ....nvim/rocks.nvim/rocks/share/lua/5.1/luarocks/loader.lua:104: error loading module 'toml_edit' from file '/home/runner/work/rocks.nvim/rocks.nvim/rocks/lib/lua/5.1/toml_edit.so':
	/lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by /home/runner/work/rocks.nvim/rocks.nvim/rocks/lib/lua/5.1/toml_edit.so)

any idea what could be going on?

@mrcjkb
Copy link
Member

mrcjkb commented Oct 31, 2024

nevermind, I've upgraded the Ubuntu runner and it's fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants