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

feat(lua): add vim.fs.abspath #28187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

famiu
Copy link
Member

@famiu famiu commented Apr 5, 2024

Problem: There is currently no way to check if a given path is absolute or convert a relative path to an absolute path through the Lua stdlib. vim.fs.joinpath does not work when the path is absolute. There is also currently no way to resolve C:foo\bar style paths in Windows.

Solution: Add vim.fs.abspath, which allows converting any path to an absolute path. This also allows checking if current path is absolute by doing vim.fs.abspath(path) == path. It also has support for C:foo\bar style paths in Windows.

(depends on #28203)

TODO: Add tests

@github-actions github-actions bot added the lua stdlib label Apr 5, 2024
@famiu famiu added the filesystem file metadata/attributes, filenames, path manipulation label Apr 5, 2024
@dundargoc dundargoc requested a review from justinmk April 5, 2024 14:46
runtime/lua/vim/fs.lua Outdated Show resolved Hide resolved
runtime/lua/vim/fs.lua Outdated Show resolved Hide resolved
@lewis6991 lewis6991 requested a review from gpanders April 5, 2024 14:59
runtime/doc/lua.txt Outdated Show resolved Hide resolved
runtime/lua/vim/fs.lua Outdated Show resolved Hide resolved
--- @param path string Path
--- @param opts vim.fs.abspath.Opts? Optional keyword arguments:
--- @return string Absolute path
function M.abspath(path, opts)
Copy link
Member

Choose a reason for hiding this comment

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

if paths is a list, that has a few advantages:

  • allows multiple segments to be combined, similar to nodejs resolve() https://nodejs.org/api/path.html#pathresolvepaths
  • when calling, the "cwd" is the first arg, which is more readable and more explicit. compare:
    • abspath('foo', {cwd: '.'})
    • abspath({'.', 'foo'})
    • making it explicit has other advantages, such as reminder plugin authors to think about getcwd() and window-local paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • allows multiple segments to be combined, similar to nodejs resolve()

vim.fs.joinpath() can do that already

  • when calling, the "cwd" is the first arg, which is more readable and more explicit. compare:when calling, the "cwd" is the first arg, which is more readable and more explicit. compare:

Tbh with you, I personally do not find it more intuitive, the opposite, even. And even if it were more intuitive, in the vast majority of cases, you're going to not set cwd manually, and in those cases, just having a single path argument is much simpler.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively could define it as path: string|table. That allows opt-in to explicit multi-arg form. Although that's a bit redundant with joinpath(), it eliminates the opts.cwd option.

in the vast majority of cases, you're going to not set cwd manually, and in those cases, just having a single path argument is much simpler.

For the single-arg case we can default to using CWD as the root dir.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively could define it as path: string|table. That allows opt-in to explicit multi-arg form. Although that's a bit redundant with joinpath(), it eliminates the opts.cwd option.

in the vast majority of cases, you're going to not set cwd manually, and in those cases, just having a single path argument is much simpler.

For the single-arg case we can default to using CWD as the root dir.

We might need to add more opts (like path OS style and whether to expand env variables) to abspath in the future, so imo keeping the opts dict is best for now, it's also more in line with the abspath function in something like Python. Having a dict just adds more complexity for the majority of usecases

Copy link
Member

Choose a reason for hiding this comment

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

We might need to add more opts (like path OS style and whether to expand env variables) to abspath in the future,

Irrelevant. The door is open for that in the future. But today, we can support a list arg as the first argument, that's natural and more readable than a CWD arg that follows the first arg.

Copy link
Member

Choose a reason for hiding this comment

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

For the record, I'm with famiu here. I don't understand the objection to the original signature at all, provided that there's a compelling use case for resolving paths relative to non-current directories. (Maybe cwd is the wrong name and base or similar would be better?) If we currently don't have such a use case -- either in our own code or a reasonably common plugin -- we can leave it for later (with the understanding that we'll simply add it and not start fighting over it again).

Copy link
Member

Choose a reason for hiding this comment

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

Should we rather implement this as an option on normalize()? Not sure that abspath() is worth all the extra docs.

Copy link
Member

@clason clason Apr 17, 2024

Choose a reason for hiding this comment

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

Apples and oranges. normalize() does not need a cwd, since its action doesn't depend on it. Only the conversion from relative to absolute path needs to know what the path is relative to.

The thing to keep in mind is that you may wish to normalize a relative path, or make a non-normalized path absolute. So it's either small, composable, single-purpose functions (the ooonix way) or one big swiss army knife function that transforms paths, controlled by (a bunch of) options you need to track interactions of.

The reason we went for the former is spelled out in the PR description: it obviates the need for a separate is_absolute function (since the conversion is lightweight enough).

Copy link
Member

Choose a reason for hiding this comment

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

The thing to keep in mind is that you may wish to normalize a relative path, or make a non-normalized path absolute.

What I meant was, similar to the expand_env option, normalize() could have a abs option.

So it's either small, composable, single-purpose functions (the ooonix way) or one big swiss army knife function that transforms paths, controlled by (a bunch of) options you need to track interactions of.

Yes, but the 2 main concepts are "normalize" and "resolve". Currently normalize() is also doing some "resolving" (the expand_env option), and arguably that should be pulled into a separate resolve() or expand() function.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway to be clear, this PR is not blocked on the above.

@famiu famiu force-pushed the feat/vim.fs/abspath branch 3 times, most recently from 086197a to b32e57d Compare April 9, 2024 18:41
@famiu famiu force-pushed the feat/vim.fs/abspath branch 6 times, most recently from 1123826 to e38e4f4 Compare April 17, 2024 09:39
@justinmk
Copy link
Member

Do we also want relpath() ? https://nodejs.org/api/path.html#pathrelativefrom-to

@famiu
Copy link
Member Author

famiu commented Apr 17, 2024

Do we also want relpath() ? https://nodejs.org/api/path.html#pathrelativefrom-to

Maybe, but personally I'd wait to see if there are any convincing usecases for that in Neovim first

@famiu famiu marked this pull request as ready for review April 17, 2024 11:14
runtime/doc/lua.txt Outdated Show resolved Hide resolved
src/nvim/lua/executor.c Outdated Show resolved Hide resolved
src/nvim/lua/fs.c Outdated Show resolved Hide resolved
Comment on lines +421 to +430
local cwd = vim.uv.cwd()
local home = vim.uv.os_homedir()

assert(cwd ~= nil)
assert(home ~= nil)

if is_os('win') then
cwd = cwd:gsub('\\', '/')
home = home:gsub('\\', '/')
end
Copy link
Member

Choose a reason for hiding this comment

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

(untested)

Suggested change
local cwd = vim.uv.cwd()
local home = vim.uv.os_homedir()
assert(cwd ~= nil)
assert(home ~= nil)
if is_os('win') then
cwd = cwd:gsub('\\', '/')
home = home:gsub('\\', '/')
end
local cwd = assert(is_os('win') and (vim.uv.cwd()):gsub('\\', '/') or vim.uv.cwd())
local home = assert(is_os('win') and (vim.uv.os_homedir()):gsub('\\', '/') or vim.uv.os_homedir())

Copy link
Member Author

@famiu famiu Apr 17, 2024

Choose a reason for hiding this comment

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

If cwd or home are nil, gsub will not work. The if statement imo is also much nicer to read and has less redundancy (e.g., two same vim.uv.os_homedir() calls on the same line)

Copy link
Member

Choose a reason for hiding this comment

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

If cwd or home are nil, gsub will not work

it will error, just like the assert(). for the purposes of this test code that's pretty equivalent. though I don't know of LuaLS will like it.

Copy link
Member Author

Choose a reason for hiding this comment

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

LuaLS does complain about it if I change it to that, it also looks very cluttered and hard to parse mentally, imo.

Copy link
Member

Choose a reason for hiding this comment

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

How is following a bunch of reassignments easier to parse mentally?

Ternaries are preferred. Not going to argue about this over and over.

test/functional/lua/fs_spec.lua Outdated Show resolved Hide resolved
src/nvim/lua/fs.c Outdated Show resolved Hide resolved
@justinmk
Copy link
Member

#20976 proposed realpath. How does it compare to abspath ? Do they have overlap? If so, maybe a resolve() or something like that makes more sense.

@echasnovski
Copy link
Member

The realpath() works only if path is present on disk and resolves symlinks. The vim.fs.abspath() was intended with the sole purpose of transforming any path to be absolute (whatever it means depending on the OS) without unnecessary extra changes to the path itself.

@justinmk
Copy link
Member

The vim.fs.abspath() was intended with the sole purpose of transforming any path to be absolute (whatever it means depending on the OS) without unnecessary extra changes to the path itself.

Doesn't fslua_get_drive_cwd kind of violate that? Because it is context-dependent.

We could have a flag that controlled the behavior. Main point is that realpath and abspath seem pretty related so having two different functions is confusing.

@famiu
Copy link
Member Author

famiu commented Apr 20, 2024

The vim.fs.abspath() was intended with the sole purpose of transforming any path to be absolute (whatever it means depending on the OS) without unnecessary extra changes to the path itself.

Doesn't fslua_get_drive_cwd kind of violate that? Because it is context-dependent.

As context-dependent as it needs to be, not any more than that. Resolving symlinks when it's not intended goes a step beyond.

We could have a flag that controlled the behavior. Main point is that realpath and abspath seem pretty related so having two different functions is confusing.

From what I understand, realpath is for resolving symlinks for existing paths, abspath is for converting relative paths to absolute paths, two very different usecases. It's also not possible to make vim.uv.fs_realpath NOT resolve symlinks or for it to work for non-existing paths, so a separate abspath functionality is needed either way.

Problem: There is currently no way to check if a given path is absolute or convert a relative path to an absolute path through the Lua stdlib. `vim.fs.joinpath` does not work when the path is absolute. There is also currently no way to resolve `C:foo\bar` style paths in Windows.

Solution: Add `vim.fs.abspath`, which allows converting any path to an absolute path. This also allows checking if current path is absolute by doing `vim.fs.abspath(path) == path`. It also has support for `C:foo\bar` style paths in Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem file metadata/attributes, filenames, path manipulation lua stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants