nvim version from env #207
Conversation
|
|
| accessSync(possibleNvimPath, constants.X_OK); | ||
| const nvimVersionFull = execSync( | ||
| `${possibleNvimPath} --version` | ||
| ).toString(); |
There was a problem hiding this comment.
nvim --api-info returns a msgpack object that includes a version dict. That avoids needing to parse nvim --version though I suppose that output isn't likely to change.
| } | ||
|
|
||
| /** | ||
| * Get the highest matching nvim version from the environment. |
There was a problem hiding this comment.
The "find the highest" version logic seems nice, but it might be worth dropping it to simplify the code. Instead, if the first nvim found doesn't meet the minimum version, the caller can simply report this to the user (mentioning the found nvim path and version).
fwiw, Nvim 0.9+ includes vim.version.parse(). Could eventually use that to avoid rolling our own semver parsing here.
|
Is it worth adding a dependency like msgpackr to use the We have no control over the code in a dependency like this. This would bypass our linter rules. I could write a more lightweight version of this that only does what we need here, but I don't think this is worth the effort. |
Not worth adding a new dependency. But maybe (separate PR) worth replacing our old dependency (which was not particularly fast)? node-client/packages/neovim/package.json Line 47 in c4ba031 |
|
The currently used |
|
Is there anything else you want me to change so this PR can be merged @justinmk ? |
| export function getNvimFromEnv( | ||
| opt: GetNvimFromEnvOptions = {} | ||
| ): Readonly<GetNvimFromEnvResult> { | ||
| const paths = process.env.PATH.split(delimiter); |
There was a problem hiding this comment.
$PATH is likely not sufficient, especially for applications such as https://github.com/vscode-neovim/vscode-neovim which are started in GUI contexts and often (macOS, at least) the user's terminal $PATH is much different than whatever GUI apps are started with.
Thus we also need to search "common locations". These are the ones I know of:
/usr/local/bin/
/usr/bin
/opt/homebrew/bin
/home/linuxbrew/.linuxbrew/bin
$HOME/.linuxbrew/bin
$HOME/bin
Ironically on Windows I think we can depend on $PATH. I assume that winget and scoop setup $PATH, which gets broadcast to GUI programs on Windows. Else we may also need to add those locations. But that could be improved later.
|
Great work, thanks! Merging, but I think the "common locations" will be needed for this to be maximally useful. #267 |
closes #146
I'm still working on it.
The version comparison is currently just a placeholder for the correct implementation.
I would appreciate any feedback to know if I'm on the right path here. @justinmk