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

Upgrade folke/neodev (sunsetting) to folke/lazydev #961

Closed
wants to merge 1 commit into from

Conversation

qgates
Copy link
Contributor

@qgates qgates commented Jun 2, 2024

Address issue #960

@VlaDexa VlaDexa mentioned this pull request Jun 5, 2024
@iton0
Copy link

iton0 commented Jun 8, 2024

Since this is a dependency of the lsp would this not be loaded along with the lsp config regardless of filetype?

-- used for completion, annotations and signatures of Neovim apis
{ 'folke/neodev.nvim', opts = {} },
{ 'folke/lazydev.nvim', ft = 'lua', opts = {} },
Copy link
Contributor

@rmacklin rmacklin Jun 20, 2024

Choose a reason for hiding this comment

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

To avoid an "undefined field fs_stat" LSP error on

if not vim.uv.fs_stat(lazypath) then

perhaps this should use the configuration suggested in https://github.com/folke/lazydev.nvim/tree/6184ebbbc8045d70077659b7d30c705a588dc62f#-installation

Suggested change
{ 'folke/lazydev.nvim', ft = 'lua', opts = {} },
{
'folke/lazydev.nvim',
ft = 'lua',
opts = {
library = {
-- Load luvit types when the `vim.uv` word is found
{ path = 'luvit-meta/library', words = { 'vim%.uv' } },
},
},
},
{ 'Bilal2453/luvit-meta', lazy = true },

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 got the sense that these are optional configs for those who want specific vim.uv LSP typings. Not everyone will need/want them. However, if there's no downside and or most people need them, I see no problem with it.

At the risk of bloat perhaps someone else can chime in?

Copy link

@gustavosinacio gustavosinacio Jun 20, 2024

Choose a reason for hiding this comment

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

I feel like, for a kickstart config, the suggested would be the preferred aproach. Anyone who doens't want this will propably be more aware of how to remove it.

Copy link
Contributor

@rmacklin rmacklin Jun 20, 2024

Choose a reason for hiding this comment

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

As it stands, on master using folke/neodev.nvim we don't see an "undefined field fs_stat" LSP error. But with the change in this PR, we do see that error (unless we apply the suggestion above). That's why I think we might want to follow the configuration suggested in https://github.com/folke/lazydev.nvim/tree/6184ebbbc8045d70077659b7d30c705a588dc62f#-installation - Otherwise, merging this PR as-is will leave the fresh kickstart.nvim installation with an LSP error in init.lua, which IMO degrades the desired "first timer" experience.

@feoh
Copy link
Collaborator

feoh commented Jul 21, 2024

Has this PR been abandoned? It seems like merging it will leave us with errors which is no bueno. Could we either fix or close? Thanks a bunch!

@rmacklin
Copy link
Contributor

@feoh This PR was cherry-picked into #936, which was just merged, so this PR can be closed.

But this also means the LSP error discussed above is present in master now. I've opened a PR with my suggestion from #961 (comment) here:

@feoh
Copy link
Collaborator

feoh commented Jul 22, 2024

Thank you so much for being so incredibly thorough. I really appreciate having the links ready at hand so I can read up on the context and understand the nature of the fix and why it's important.

Closing this, merging that :)

@feoh feoh closed this Jul 22, 2024
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.

None yet

5 participants