-
-
Notifications
You must be signed in to change notification settings - Fork 625
fix(#2507): icon in message after rename-file #2510
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
FYI: you don't need to open a new PR when you want to change something, you can just add another commit to the branch. |
ok thank you. should i close PR and commit again? |
No, if some changes are needed just push a new commit to your |
@@ -53,7 +53,9 @@ function M.fn(default_modifier) | |||
|
|||
-- support for only specific modifiers have been implemented | |||
if not ALLOWED_MODIFIERS[modifier] then | |||
return notify.warn("Modifier " .. vim.inspect(modifier) .. " is not in allowed list : " .. table.concat(ALLOWED_MODIFIERS, ",")) | |||
return notify.warn( | |||
string.format("Modifier %s is not in allowed list: %s", vim.inspect(modifier), table.concat(ALLOWED_MODIFIERS, ",")) |
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.
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 am sorry. I understood gegoune's words to fix that as well.
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, now that I think about it, I'm not sure if the "same here" is about the unrelated style changes or the use of string.format
... Let's wait for his feedback.
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.
Okay and I finally understand your FYI. Thank you for your kindness.
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.
Apologies if I wasn't clear enough. My fault.
Regarding unrelated changes: I meant it for formatting changes toward the top of file.
string.format
is what we should be using going forward but as @Akmadan23 said existing code should not be changed as part of that pr to make it self contained. We can replace other string concatenations across code base in separate pr (I can do it).
Hope that clears things up. Thanks for your pr!
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.
Many thanks for your contribution!
Compatibility with all terminals is desirable.
Closes #2507