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

fs.upwards-file-search not always returning absolute paths #587

Closed
pyrmont opened this issue Jul 6, 2024 · 4 comments
Closed

fs.upwards-file-search not always returning absolute paths #587

pyrmont opened this issue Jul 6, 2024 · 4 comments
Labels
bug Something isn't working fixed-pending-release

Comments

@pyrmont
Copy link
Contributor

pyrmont commented Jul 6, 2024

In the docstring for fs.upwards-file-search, it says:

(defn upwards-file-search [file-names from-dir]
"Given a list of relative filenames and an absolute path to a directory,
return the absolute path of the first file that matches a relative path,
starting at the directory and then upwards towards the root directory. If no
match is found, return nil."

I don’t think this is always correct. In my code, if the file is in the from-dir directory, what will be returned is the bare filename rather than the absolute path. I think this is because that’s what nvim.fn.findfile returns.

@Olical Olical added the bug Something isn't working label Jul 14, 2024
@Olical
Copy link
Owner

Olical commented Jul 14, 2024

Thanks for catching this, I've updated the chain of functions based on findfile to now resolve paths to their absolute versions and then normalize them for the OS we're working on. I worry that this will break Windows support somehow as it usually does whenever I do anything path related 😢

I really need to get the test suite running under Windows like what is currently progressing in nfnl, but there's some Neovim Windows bugs around file trust stuff that needs to be fixed first. I think I won't port this system to Windows but instead include that as part of the nfnl port.

I'm wondering if I should normalize here, maybe just expanding to absolute paths is enough and the normalize step will be what breaks Windows support. I don't have a way to test this right now though, I have no idea what steps it would take to test this out on Windows.

@Olical
Copy link
Owner

Olical commented Jul 14, 2024

Oh and the tests are failing in CI but not locally, drat!

@Olical
Copy link
Owner

Olical commented Jul 14, 2024

Removed the normalize call, I think that's good in nfnl land but might break assumptions in Conjure around how we treat paths in the system.

@pyrmont
Copy link
Contributor Author

pyrmont commented Jul 19, 2024

Thanks! Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed-pending-release
Projects
None yet
Development

No branches or pull requests

2 participants