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

refactor!: deprecate util.root_pattern #3569

Closed
wants to merge 1 commit into from

Conversation

dundargoc
Copy link
Member

@dundargoc dundargoc commented Jan 19, 2025

The primary (and potentially breaking) change is that
util.root_pattern traverses once per provided file pattern, while
vim.fs.find only traverses once. This means that the search will no
longer be prioritized by file order, which may break user configurations
who unkowingly rely on this behavior.

This will also be breaking for language servers that rely on code
dependencies that is archived but still needs to be read. More context:
#1687.

Work on #2079.

@dundargoc
Copy link
Member Author

dundargoc commented Jan 19, 2025

I couldn't come up with a good way to replicate the functionality provided by strip_archive_subpath so think we might just want to cut our losses here. @mfussenegger had a big brain suggestion about a related lsp feature that did something similar (by being able to read code from any URI or something?), so might want to use that in the future in core instead.

Found it: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.18/specification/#workspace_textDocumentContent

@dundargoc dundargoc force-pushed the refactor/root_pattern branch 17 times, most recently from 23b1228 to 33a5cb3 Compare January 20, 2025 13:11
@dundargoc dundargoc marked this pull request as ready for review January 20, 2025 13:12
@dundargoc dundargoc requested a review from glepnir as a code owner January 20, 2025 13:12
doc/lspconfig.txt Outdated Show resolved Hide resolved
@dundargoc dundargoc force-pushed the refactor/root_pattern branch 3 times, most recently from 6b02073 to 1a34a23 Compare January 20, 2025 13:59
@dundargoc dundargoc requested a review from justinmk January 20, 2025 17:18
Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

Before merging this, let's wait a ~day to see if master is stable enough to tag a release.

Also: hoping that once we bump the minimum Nvim to 0.10, we can migrate these cases to vim.fs.root. If vim.fs.root doesn't work then we need to find out what it's missing ASAP :)

@dundargoc
Copy link
Member Author

Looking more closely: I think vim.fs.find and util.root_pattern behaves differently in that find traverses once and root_pattern traverses once per provided filepattern, meaning that the ordering of the files used to matter. This would be a breaking change for users/configs unknowingly relying on this behavior, but it's impossible to say beforehand which servers it'd be without user input. I suggest we merge this anyway so we can at least evaluate the actual effect out there and react accordingly.

I'll add this info to the commit message as well.

@dundargoc dundargoc force-pushed the refactor/root_pattern branch from 1a34a23 to d95c5d9 Compare January 21, 2025 15:04
The primary (and potentially breaking) change is that
`util.root_pattern` traverses once per provided file pattern, while
`vim.fs.find` only traverses once. This means that the search will no
longer be prioritized by file order, which may break user configurations
who unkowingly rely on this behavior.

This will also be breaking for language servers that rely on code
dependencies that is archived but still needs to be read. More context:
neovim#1687.

Work on neovim#2079.
@dundargoc dundargoc force-pushed the refactor/root_pattern branch from d95c5d9 to 3065004 Compare January 21, 2025 15:06
@justinmk
Copy link
Member

justinmk commented Jan 21, 2025

meaning that the ordering of the files used to matter.

Er, that's very much intentional: #2885

Changing it to "random" would be even worse than reverting that PR.

but it's impossible to say beforehand which servers it'd be without user input.

There was quite a lot of user input, and PRs, on this topic (unless I'm mistaken about #2885 being related).

Related?:

@dundargoc
Copy link
Member Author

dundargoc commented Jan 21, 2025

What about the following?

  1. Add option in vim.fs.find for this functionality. As an example, python's os.walk has a topdown that adjust whether the traversal is breadth first or depth first. A similar option for vim.fs.find would suit it well I think.
  2. Make it depth first by default for vim.fs.root so order matters.

This approach seems like the simplest way to move forward, but I understand if you're hesitant to add more options to vim.fs.find.

Edit: in the meantime, this problem can be worked around by doing multiple calls to vim.fs.find. Aka find(priority1) or find(priority2) or .... It's clunky but not super awful.

@justinmk
Copy link
Member

What about the following?

Those SGTM.

Edit: in the meantime, this problem can be worked around by doing multiple calls to vim.fs.find. Aka find(priority1) or find(priority2) or .... It's clunky but not super awful.

Maybe we should keep root_pattern around for now. There's no real reason to avoid it until we have an answer in core.

@dundargoc
Copy link
Member Author

Oof. I understand.

Closing for now until there's a solution in core as described in #3569 (comment).

@dundargoc dundargoc closed this Jan 21, 2025
@dundargoc dundargoc deleted the refactor/root_pattern branch January 21, 2025 15:42
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