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

fix: version numbers in code actions #306

Closed
wants to merge 2 commits into from
Closed

fix: version numbers in code actions #306

wants to merge 2 commits into from

Conversation

bustercopley
Copy link
Contributor

@bustercopley bustercopley commented Oct 19, 2023

Zulip topic

VS Code ignores the version number in a WorkspaceEdit but does not allow it to be null. There is at least one LSP client which does validate the version number, namely the Eglot client included in Emacs. By sending the correct version, we can be compatible with both.

This is a self-contained (std4-only) version, instead of the two changes (to lean4 and std4) that I mentioned on Zulip.

@bustercopley
Copy link
Contributor Author

Fixed unused variable linter errors

@joehendrix
Copy link
Contributor

@bustercopley Thanks, I'll find the right reviewer for this so we can figure out if it should belong in std or core.

@bustercopley
Copy link
Contributor Author

Thanks!
Fixed long line linter error.

@david-christiansen
Copy link
Contributor

From my reading of the spec, providing a null version means that the server has not yet heard of the file being open, rather than that the server simply doesn't want to send it, so this seems to strictly increase the conformance to the protocol.

Std/CodeAction/Basic.lean Outdated Show resolved Hide resolved

namespace Lean.Lsp.WorkspaceEdit

/-- Construct a WorkspaceEdit from a VersionedTextDocumentIdentifier and a TextEdit --/
Copy link
Member

@digama0 digama0 Oct 20, 2023

Choose a reason for hiding this comment

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

This should be in Std.Lean, maybe Std.Lean.Lsp, ditto for textDocument

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it may be better off in core as originally suggested by @bustercopley. Alternatively, if we keep it in std, we should delete ofTextEdit in core so that downstream users do not accidentally use the wrong function. I think I would personally prefer the former.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, especially since the changes to core amount to a simplificiation/streamlining, that is the natural place to do it. (I'm a bit intimidated by the process for core Lean PRs.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, especially since the changes to core amount to a simplificiation/streamlining, that is the natural place to do it. (I'm a bit intimidated by the process for core Lean PRs.)

It's fine to PR this without a corresponding RFC since it is very small, uncontroversial and you discussed it with the Lean community and the respective code owner :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhuisi :) Do you mind saying something along those lines in the Zulip topic?

Std/CodeAction/Basic.lean Outdated Show resolved Hide resolved
@bustercopley
Copy link
Contributor Author

Force push: now depends on core #2721; squashed and rebased.

@bustercopley bustercopley changed the title fix: use version number from EditableDocument for WorkspaceEdit fix: version numbers in code actions Oct 20, 2023
@kim-em kim-em added the depends on core changes This PR need only be reviewed after changes land in Lean core. label Oct 21, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Oct 21, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. and removed merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. labels Oct 21, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. and removed merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. labels Oct 22, 2023
@kim-em kim-em changed the title fix: version numbers in code actions fix: version numbers in code actions [merged into nightly-testing] Oct 25, 2023
kim-em added a commit that referenced this pull request Oct 25, 2023
@kim-em kim-em added v4.3.0-rc1 merged-into-nightly-testing This has been merged into `nightly-testing`, and will join `main` when we update the toolchain. labels Oct 25, 2023
@kim-em kim-em changed the title fix: version numbers in code actions [merged into nightly-testing] fix: version numbers in code actions Oct 25, 2023
@digama0 digama0 closed this in 0daeae3 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on core changes This PR need only be reviewed after changes land in Lean core. merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. merged-into-nightly-testing This has been merged into `nightly-testing`, and will join `main` when we update the toolchain.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants