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

docs(readme): make bootstrap snippet work well with lua-language-server #617

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

saccarosium
Copy link
Contributor

Hi,
this PR tries to:

  • make the bootstrapping snippet work a bit nicer with lua-language-server. lua lsp will complain about the use of vim.fn.stdpath in vim.fs.joinpath since it can return string[] and will throw a warning. The fix is telling it to treat the stdpath call as a string.
  • make the bootstrapping snippet less intimidating by reducing is sice in LOC
  • remove redundant vim.fs.normalize call (vim.fs.joinpath already does that`)
  • move the vim.v.shell_error check in the same branch as a "sanity check". It is nitpicking but, as is it right now, this can be trigger by any vim.fn.system or :! gone wrong in the user configuration and moving it to the same branch as the vim.fn.system call that clones the repo this can make life much saner to user.

Copy link
Contributor

Review Checklist

Does this PR follow the Contribution Guidelines? Following is a partial checklist:

Proper conventional commit scoping:

  • For example, fix(installer): some installer bugfix

  • Pull request title has the appropriate conventional commit prefix.

If applicable:

  • Tested
    • Tests have been added.
    • Tested manually (steps in PR description).
  • Updated documentation.

Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

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

Hey 👋
Thanks for your PR 🙏

remove redundant vim.fs.normalize call (vim.fs.joinpath already does that`)

That is incorrect. joinpath (as of writing) only does gsub('//+', '/'), which is less than normalize.

Please also revert your change to the joinpath arguments. Right now, joinpath adds a / on all platforms, but that could change.

Please also refer to the review checklist, specifically regarding the use of conventional commits.

@saccarosium
Copy link
Contributor Author

That is incorrect. joinpath (as of writing) only does gsub('//+', '/'), which is less than normalize.

Interesting, I was basing this on the documentation. I didn't actually read the implementation. I should have done it...

Concatenate directories and/or file paths into a single path with
normalization (e.g., "foo/" and "bar" get joined to "foo/bar")
-- :h vim.fs.joinpath

@saccarosium saccarosium force-pushed the master branch 2 times, most recently from 109c3b4 to e50037e Compare December 28, 2024 09:46
@mrcjkb mrcjkb changed the title Make bootstrap snippet works well with lsp docs(readme): make bootstrap snippet work well with lua-language-server Jan 2, 2025
@mrcjkb mrcjkb merged commit 0be83ec into nvim-neorocks:master Jan 2, 2025
7 checks passed
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