-
Notifications
You must be signed in to change notification settings - Fork 123
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
Compatibility with Merlin 503 preview #1386
base: master
Are you sure you want to change the base?
Conversation
ocaml-lsp-server.opam
Outdated
@@ -31,7 +31,6 @@ depends: [ | |||
"dyn" | |||
"stdune" | |||
"fiber" {>= "3.1.1" & < "4.0.0"} | |||
"ocaml" {>= "5.2.0"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. Don't we need a lower bound based on the stdlib/language features we're using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I would expect that lower-bound to coincides with the one imposed by the compatible merlin-lib
versions. Not specifying the lower-bound on OCaml here means Opam CI will fail if we don't respect that invariant.
I don't mind adding it back of course if that's a better practice.
For the record: I plan to do releases of Merlin and LSP for 5.2 next week, and for 5.3 around the time of the final release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to always specify direct deps as precisely as possible and not to rely on transitive deps. For example, what if we start using a new language feature or a new function. Our lower bound might be higher than merlin's? Conversely, what if merlin expands its support to older versions of OCaml that we can't build against?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our lower bound might be higher than merlin
This is something we want to avoid for releases of LSP to be compatible with as many versions as merlin-lib as possible (which is only 5.2 and 5.3 these days anyway).
I think it makes sense to always specify direct deps as precisely as possible and not to rely on transitive deps.
Works for me, I will make the change.
2d66db7
to
e266e93
Compare
No description provided.