Skip to content
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

Small nitpipicks #608

Closed
wants to merge 1 commit into from
Closed

Conversation

saccarosium
Copy link
Contributor

Hi,
this are some small nitpicks that IMHO make the code a bit more clear.

  • You can simply pass the functions to vim.schedule_wrap
  • Why do we need allowed_default_*_opts globally when we are using them only in one function?
  • You can force the lsp to cast a value instead of creating another function
  • Use tbl_isempty instead of checking the legth with #

@github-actions github-actions bot requested a review from stevearc December 25, 2024 10:46
@stevearc
Copy link
Owner

stevearc commented Jan 3, 2025

I understand that this PR is likely coming from a place of using the plugin, liking it, and wanting to contribute. I want you to know that that is a good instinct and I appreciate the thought behind it. With that said, this is not a useful PR and I would generally advise you against making these types of PRs. Every PR takes some amount of my time and energy as a maintainer, and what I hope to get out of it is that the benefit to the project is worth that input. The benefits to a nitpick PR are very small, subjective, and often a matter of taste.

Another reason why I would generally advise against this is that you're unfamiliar with the project and something that seems oddly verbose for no reason might actually have a reason. For example, we're not just calling vim.schedule_wrap(vim.notify) because vim.notify can get patched to be another function, and we want to have the latest version of vim.notify not the version that existed when this file was required. Some of the other nits you changed have similar, subtle reasons for why they are as they are.

So I will not be merging this, but if you want to contribute some other way in the future, say by adding a formatter or fixing a bug, I would love to look at another PR!

@stevearc stevearc closed this Jan 3, 2025
@saccarosium
Copy link
Contributor Author

saccarosium commented Jan 3, 2025 via email

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.

2 participants