Skip to content

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.

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