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

vscode snippet loader ignores paths after invalid path #727

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

bootleq
Copy link
Contributor

@bootleq bootleq commented Jan 17, 2023

Hi, the loader with specific paths might behave unexpectedly, where first invalid path make further items ignored.
For example:

require('luasnip.loaders.from_vscode').load({
  paths = {
    '/NOT/EXIST/PATH',  -- this directory doesn't exist
    '/path/to/friendly-snippets',
  },
})

should still find friendly-snippets path, while it doesn't, all paths were ignored and not loaded.

Can also reproduce in my forked test, thanks.

@L3MON4D3
Copy link
Owner

Thank you for opening issue+solution, very nice!!
I think this is an okay solution, but I'd rather have the list passed to deduplicate not have missing items (such that it is continuous and can be iterated using ipairs).
Would you look into making that change instead?

@bootleq
Copy link
Contributor Author

bootleq commented Jan 18, 2023

Thanks for suggestion, I've changed the implementation.
Also found other loaders (lua, snipmate) have the same problem, so handled them, too.

If the changes look ok, I can to do a rebase if needed.

@L3MON4D3
Copy link
Owner

Looks perfect 👍
Squashing into one commit seems appropriate, would be great if you could do that

Loader `paths` can be a list, invalid path within (e.g., the directory
doesn't exist) will be omitted, and other valid items should be kept.

There is a problem that valid paths AFTER a bad one are omitted, too.

The cause is after normalize paths, invalid item become nil hole, then
`ipairs` in deduplicate function unexpectedly stopped there.

Solution: remove nil before dedup.

Small note: lua and snipmate loaders share
`loader.util.get_load_paths_snipmate_like` helper thus fixed together.

However vscode loader has its own `package.json` parsing logic so can't
be treated the same.
@bootleq
Copy link
Contributor Author

bootleq commented Jan 18, 2023

I've finished the rebase.
Please let me know if need change, thanks a lot.

@L3MON4D3
Copy link
Owner

No more changes, this is great :D
Thank you very much for the fix!!

@L3MON4D3 L3MON4D3 merged commit af60ac1 into L3MON4D3:master Jan 18, 2023
@bootleq bootleq deleted the loader-dedup-paths branch January 19, 2023 07:45
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