-
-
Notifications
You must be signed in to change notification settings - Fork 263
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)luarocks: make rocks path configurable; fixes #672 #957
base: master
Are you sure you want to change the base?
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.
Thanks for this; it's a good improvement. I would like to request a small change: the configuration logic can be cleaned up a bit by making the paths fields in the packer.luarocks
config
table, and using vim.tbl_extend
.
lua/packer/luarocks.lua
Outdated
local hererocks_install_dir = util.join_paths(rocks_path, lua_version.dir) | ||
local shell_hererocks_dir = vim.fn.shellescape(hererocks_install_dir) | ||
local config = nil | ||
local cache_path = nil |
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.
Slight nitpick: if we're making these externally configurable, let's group them together in a table to make them more manageable within this file. Really, I suppose they should just be fields in the config
table, and we should use vim.tbl_extend
with the appropriate merge behavior.
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.
Well, I'll try to invent something here, but it wouldn't be so easy: most parameters requires values of previously-defined value.
Current behaviour is just to check if it is defined in config
, and use defined value if it is, and some default value if don't. And, as it is executed line-by-line, if some "early" parameter was defined in config, it will affect all next-defined things.
And if we'll just use a table, this magic won't work, as we cant call neither other fields in the table of "defaults", nor values from not-yet-merged resulting table.
So, I'll still need to check values from _config, or invent some other way.
// by the way, I found that cache_path
here is never used in anu way except being a part of rocks_path
, so I removed it as useless.
Hope, you'll not object that change.
Also sorry for taking so long to get to this! I haven't had much time/energy for |
Well, I'll try to invent something here, but it wouldn't be so easy: most parameters requires values of previously-defined value. Current behaviour is just to check if it is defined in config, and use defined value if it is, and some default value if don't. And, as it is executed line-by-line, if some "early" parameter was defined in config, it will affect all next-defined things. And if we'll just use a table, this magic won't work, as we cant call neither other fields in the table of "defaults", nor values from not-yet-merged resulting table. So, I'll still need to check values from _config, or invent some other way. // by the way, I found that cache_path here is never used in anu way except being a part of rocks_path, so I removed it as useless. Hope, you'll not object that change. |
@wbthomason ping? (direct ping in case you're overwhelmed by notifications) |
Well, actually, it makes all paths configurable (just in case), but main reason was to make rocks path configurable, as I have
~/.cache
on tmpfs (and it is not persistent across reboots. Moreover, actually,cache_dir
by itself, wherever it placed, is not guaranteed to be persistent). So, it was impossible for me to properly use rocks functionality with hardcoded rocks path...