Skip to content

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

Merged
merged 3 commits into from
Nov 5, 2023
Merged

fix(#2507): icon in message after rename-file #2510

merged 3 commits into from
Nov 5, 2023

Conversation

Dongyomi
Copy link
Contributor

@Dongyomi Dongyomi commented Nov 4, 2023

Closes #2507

@Akmadan23
Copy link
Collaborator

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.

@Dongyomi
Copy link
Contributor Author

Dongyomi commented Nov 4, 2023

ok thank you. should i close PR and commit again?

@Akmadan23
Copy link
Collaborator

No, if some changes are needed just push a new commit to your test branch.

@@ -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, ","))
Copy link
Collaborator

Choose a reason for hiding this comment

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

As @gegoune said in #2508, this is a change outside of the scope of your PR. Using string.format here is arguably better, but it should be done in a separate refactoring.

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 am sorry. I understood gegoune's words to fix that as well.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

Copy link
Member

@alex-courtis alex-courtis left a 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.

@alex-courtis alex-courtis merged commit 20a0707 into nvim-tree:master Nov 5, 2023
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.

Icon in message after rename file
4 participants