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: improved the nvim version check #1527

Merged

Conversation

xiyaowong
Copy link
Collaborator

No description provided.

@xiyaowong xiyaowong merged commit a42a60e into vscode-neovim:master Oct 13, 2023
8 checks passed
@xiyaowong xiyaowong deleted the feat/improve-version-check branch October 13, 2023 06:43
Comment on lines +345 to +346
if (missOptions.length) errMsgs.push("Miss options: " + missOptions.join(", ") + ". ");
if (missFunctions.length) errMsgs.push("Miss functions: " + missFunctions.join(", ") + ". ");
Copy link
Collaborator

@justinmk justinmk Oct 13, 2023

Choose a reason for hiding this comment

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

s/Miss/missing

Not sure this message is helpful though. It's a roundabout way of saying "you need version X". It's simpler to just have a minimum version that we know contains the features required by vscode-neovim.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be useful and users using the dev version can understand this information.
For example, the 0.9.0-dev version used in #1524

Copy link
Collaborator

@justinmk justinmk Oct 13, 2023

Choose a reason for hiding this comment

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

For example, the 0.9.0-dev version used in #1524

But 0.9.0-dev should fail a check against 0.9.0. That's defined by semver. Whenever a prerelease tag (-dev) is present, it is strictly "less than" the release version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if -dev contains the features we need?

Copy link
Collaborator

@justinmk justinmk Oct 13, 2023

Choose a reason for hiding this comment

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

Why is it worth having complicated code (which must also be continually updated based on functions used by vscode-neovim) to support that edge case?

Nvim is intentionally "coarsely" versioned (as opposed to Vim, which has per-commit version numbers). There is zero reason to support users that happen to be on a random dev build.

Nvim during development is not stable. For example, when nvim_exec2 was added its signature may have changed in a later commit, so even if you check for the presence of nvim_exec2 that doesn't guarantee that you have the required version.

Copy link
Collaborator Author

@xiyaowong xiyaowong Oct 13, 2023

Choose a reason for hiding this comment

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

That makes sense. Learned a lot from your detailed reply

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.

None yet

3 participants