-
-
Notifications
You must be signed in to change notification settings - Fork 620
feat(#2270): option to use item names with fs action notifications #2280
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
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.
Looking great, thank you!
I'll give it a full test after changes have been addressed.
|
||
local function create_and_notify(file) | ||
local ok, fd = pcall(vim.loop.fs_open, file, "w", 420) | ||
if not ok then | ||
notify.error("Couldn't create file " .. file) | ||
local notify_file = M.config.notify.absolute_path and file or utils.get_last_path_elem(file) |
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.
We are doing this a lot; could we extract a function to do this? utils.lua
would be a good place.
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.
Yea absolutely. If I understand correctly though, the function should take in a boolean and an absolute path, and return the last element or the path itself based on the boolean value?
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 function should take in a boolean and an absolute path
Ah yes, you are right. That's not very workable.
How about we put it in notify itself? That knows about opts.notify
. The function could then just take the path and maybe strip.
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.
Hmm, I see, maybe be can do something like this then?
for _, x in ipairs(modes) do
M[x.name] = function(msg, src, dest, ...)
local args = { ... }
local parse_path = function(path)
return config.absolute_path and path or vim.fn.fnamemodify(path, ":t")
end
if src ~= nil and dest ~= nil then
msg = string.format(msg, parse_path(src), parse_path(dest), unpack(args))
elseif src ~= nil then
msg = string.format(msg, parse_path(src), unpack(args))
elseif dest ~= nil then
msg = string.format(msg, parse_path(dest), unpack(args))
else
msg = string.format(msg, unpack(args))
end
return dispatch(x.level, msg)
end
end
New Params
src
-> some element path
dest
-> another element path (primarily to be used with rename action)
{...}
-> other arguments to be used to format the msg
I believe this could potentially be used for various different use cases.
- Notifications with only source element but other format args e.g.
notify.error("Could not create %s, because %s", node.absolute_path, nil, some_err)
- Only destination and other format args e.g.
notify.error("Destination folder %s doesn't exist, try %s", nil, some_path, some_advice)
(Although probably not the most useful use case) - With both source and destination and other args e.g.
notify.error("Could not move %s into %s, because %s", node.absolute_path, to, some_err)
- Just source element e.g.
notify.info("%s was properly created", node.absolute_path)
- Just other args e.g.
notify.error("Something went wrong... %s", nil, nil, some_err)
- And other use cases.
But most importantly I think this won't be a breaking change, because you would still be able to pass in only a msg
and have the same functionality as before?
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 might be missing something, but that's probably a bit complex.
I was thinking something like this, that open-file and friends could call and pass to notify.info
.
function M.render_path(path)
if M.config.notify.absolute_path then
return fnamemodify ...
else
return path
end
end
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.
Ah! okay, yea I was definitely over complicating things here. If I understand correctly though, have render_path
be a method of notify module, and have the fs actions call upon notify.render_path(some_path)
to retrieve the appropriate path, and then be able to use it in notify.some_mode
?
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.
Yes please. Simplest/most readable solution.
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'm a fan of this solution and it works great! However, I've made an error when squashing my commits (I had way too many unnecessary commits), so I will close this PR, and raise a cleaner one if that's okay? Sorry about that.
Co-authored-by: Alexander Courtis <[email protected]>
Co-authored-by: Alexander Courtis <[email protected]>
Co-authored-by: Alexander Courtis <[email protected]>
node. These are mostly going to be useful for implementing lsp file operation actions. Co-authored-by: Alexander Courtis <[email protected]>
fixes #2270
I've tested this new option with create, rename, copy-paste, and remove file system actions, and they are working great for me. However, please do check if everything works as indented.
Also... I just noticed I've changed ➜ arrow within the rename notification to because using neovim on powershell with a ttf font doesn't have the best support for wider glyphs. But this was not intended to be in this feature implementation, if this is something that will be a breaking change for some, I'm more than happy to revert to "➜" and submit a feature request to add an option to let users change that specific arrow?