fix: remove intrusive version checking #859
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ulugbekna
reviewed
Feb 13, 2022
rgrinberg
force-pushed
the
ps/rr/fix__remove_intrusive_version_checking
branch
from
f30d3e3
to
7894d1e
Compare
ulugbekna
reviewed
Feb 14, 2022
Comment on lines
+67
to
+78
| [ ( (4, 12) | ||
| , [| "1.5.0" | ||
| ; "1.6.0" | ||
| ; "1.6.1" | ||
| ; "1.7.0" | ||
| ; "1.8.0" | ||
| ; "1.8.1" | ||
| ; "1.8.2" | ||
| ; "1.8.3" | ||
| ; "1.9.0" | ||
| |] ) | ||
| ; ((4, 13), [| "1.9.1"; "1.10.0"; "1.10.1" |]) |
There was a problem hiding this comment.
nit: this could use comments or names, eg ocaml_4_12 = (4, 12)
There was a problem hiding this comment.
Btw, ocamlc version library should have values corresponding to ocamlc versions, ie we could use Ocaml_version.v4_12 instead of (4, 12)
There was a problem hiding this comment.
ie we could use Ocaml_version.v4_12 instead of (4, 12)
This could work, but is it really clarifying anything? We are mapping version OCaml versions prefixes to ocamllsp versions.
I'll add some comments
src/ocaml_lsp.ml
Outdated
| "The extension was unable to parse `ocamllsp` version. That's \ | ||
| strange. Consider filing an issue on the project GitHub with the \ | ||
| version of your `ocamllsp`." | ||
| "The current version of ocamllsp is out of date. Consider \ |
There was a problem hiding this comment.
Suggested change
| "The current version of ocamllsp is out of date. Consider \ | |
| "There is a newer version of `ocamllsp` available. Consider \ |
"out of date" means it may not be supported already?
There was a problem hiding this comment.
Can you confirm that we can use markdown in the messages? I believe you said it was plaintext only earlier.
There was a problem hiding this comment.
It’s not formatted as markdown, but marking the binary name in the message
- Conforms to other uses in the extension
- Makes the message easier to read imho
rgrinberg
force-pushed
the
ps/rr/fix__remove_intrusive_version_checking
branch
from
7894d1e
to
4e3788f
Compare
Only notify users when all of the following conditions hold: * The running version of ocamllsp is "official". We don't offer suggestions for development versions. * We know something about the user's version ocamllsp. If the version isn't recognized by us, we do not second guess the user. * We are certain the user is able to upgrade to a newer version. If the user is forced to an older version of lsp, we allow him to use the plugin without notifications. ps-id: FC109E94-B57C-4226-9AAE-512F45524A91
rgrinberg
force-pushed
the
ps/rr/fix__remove_intrusive_version_checking
branch
from
4e3788f
to
bcaf28a
Compare
|
It’s not formatted as markdown, but marking the binary name in the message
Why don't we just use normal quotes then? Backticks are weird in plaintext.
…On Feb 14, 2022, 12:19 PM -0700, Ulugbek Abdullaev ***@***.***>, wrote:
@ulugbekna commented on this pull request.
In src/ocaml_lsp.ml:
> - %s. Consider upgrading OCaml version used in the current sandbox."
- (Ocaml_version.to_string v)
- | `Unexpected `Language_server_isn't_ocamllsp ->
- "Using a language server besides `ocamllsp` isn't expected by this \
- extension. Please, switch to using `ocamllsp` by installing the \
- package `ocaml-lsp-server` in your current sandbox."
- | `Unexpected `Missing_serverInfo
- | `Unexpected `ServerInfo_version_missing ->
- "The extension expected the server version to be sent by `ocamllsp`. \
- It is missing. Please, consider upgrading the package \
- `ocaml-lsp-server`."
- | `Unexpected `Unable_to_parse_version ->
- "The extension was unable to parse `ocamllsp` version. That's \
- strange. Consider filing an issue on the project GitHub with the \
- version of your `ocamllsp`."
+ "The current version of ocamllsp is out of date. Consider \
It’s not formatted as markdown, but marking the binary name in the message
1. Conforms to other uses in the extension
2. Makes the message easier to read imho
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you modified the open/close state.Message ID: ***@***.***>
|
|
Normal quotes are fine with me
Ulugbek
On Mon, 14 Feb 2022 at 22:49, Rudi Grinberg ***@***.***>
wrote:
… > It’s not formatted as markdown, but marking the binary name in the
message
Why don't we just use normal quotes then? Backticks are weird in plaintext.
On Feb 14, 2022, 12:19 PM -0700, Ulugbek Abdullaev ***@***.***>, wrote:
> @ulugbekna commented on this pull request.
> In src/ocaml_lsp.ml:
> > - %s. Consider upgrading OCaml version used in the current sandbox."
>
> - (Ocaml_version.to_string v)
>
> - | `Unexpected `Language_server_isn't_ocamllsp ->
>
> - "Using a language server besides `ocamllsp` isn't expected by this \
>
> - extension. Please, switch to using `ocamllsp` by installing the \
>
> - package `ocaml-lsp-server` in your current sandbox."
>
> - | `Unexpected `Missing_serverInfo
>
> - | `Unexpected `ServerInfo_version_missing ->
>
> - "The extension expected the server version to be sent by `ocamllsp`. \
>
> - It is missing. Please, consider upgrading the package \
>
> - `ocaml-lsp-server`."
>
> - | `Unexpected `Unable_to_parse_version ->
>
> - "The extension was unable to parse `ocamllsp` version. That's \
>
> - strange. Consider filing an issue on the project GitHub with the \
>
> - version of your `ocamllsp`."
>
> + "The current version of ocamllsp is out of date. Consider \
>
> It’s not formatted as markdown, but marking the binary name in the
message
>
> 1. Conforms to other uses in the extension
> 2. Makes the message easier to read imho
>
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
—
Reply to this email directly, view it on GitHub
<#859 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD4YR67CBKBJ4N7GWQKHXIDU3F2F5ANCNFSM5OHM7XZQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only notify users when all of the following conditions hold:
The running version of ocamllsp is "official". We don't offer
suggestions for development versions.
We know something about the user's version ocamllsp. If the version
isn't recognized by us, we do not second guess the user.
We are certain the user is able to upgrade to a newer version. If the
user is forced to an older version of lsp, we allow him to use the
plugin without notifications.