search

flycheck / flycheck-ocaml

OCaml support for Flycheck using Merlin
GNU General Public License v3.0
22 stars 6 forks source link

Update for merlin 3.0 #7

Closed drvink closed 6 years ago

drvink commented 6 years ago

This also updates the tests, which I have ran with success locally.

drvink commented 6 years ago

Collapsing whitespace in the messages made them basically unreadable if they weren't one-liners to begin with, so I've also removed that.

cpitclaudel commented 6 years ago

Thanks. Before I merge this, can you give me a better sense of what the backwards compatibility story is?

AFAICT, the recent merlin update broke this package, so anyone who runs opam update will get a broken flycheck-ocaml until this is merged; correct?

What about people who don't update merlin immediately? Will this break their Emacs?

(Small request: please let us know a bit earlier when this kind of changes happen, so we can proceed smoothly instead of breaking and fixing things)

drvink commented 6 years ago

This PR works with merlin 3.x. It will not work with merlin 2.x. Your feedback is welcome here, as I'm not sure what the best solution is.

If you upgrade the merlin elisp from 2.x to 3.x at present (i.e. prior to this PR being merged), flycheck-ocaml breaks. If you upgrade the merlin elisp (because you prefer to manage it via package.el and MELPA) without upgrading the merlin binary or vice versa, you get a broken merlin and flycheck-ocaml breaks.

We could detect which merlin elisp package is installed and use either the old mechanism or the new mechanism accordingly. However, nearly everyone is going to install flycheck-ocaml via package.el. What version do we specify as the minimum for the merlin elisp dependency? Furthermore, what if their merlin elisp is loaded from their OPAM installation instead of having been installed via package.el? Now they try to install flycheck-ocaml and they get multiple merlin elisp bundles (since we surely want to declare that we depend on the merlin elisp package), and whichever one is on the load-path first wins--I don't like this at all.

The ideal thing would seem to be, though we could worry about this later, to get flycheck-ocaml adopted as part of the merlin elisp bundle so that we don't exist in our present inconvenient form as something that doesn't integrate very well with either merlin itself or OPAM. I would imagine the merlin maintainers would be OK with this, given that merlin already has optional dependencies on auto-complete and company: it doesn't declare them as package.el requirements, so if you don't have them installed at byte-compilation time, those files simply fail to be compiled--we would simply be another such optional dependency.

(I am not a merlin maintainer--just a happy OCaml/flycheck/merlin user! I imagine they don't use this package, since merlin already has its own stuff for handling errors and drawing overlays, but theirs doesn't integrate with flycheck.)

cpitclaudel commented 6 years ago

If you upgrade the merlin elisp (because you prefer to manage it via package.el and MELPA) without upgrading the merlin binary or vice versa, you get a broken merlin and flycheck-ocaml breaks.

Yeah, I saw that :/

(I am not a merlin maintainer--just a happy OCaml/flycheck/merlin user! I imagine they don't use this package, since merlin already has its own stuff for handling errors and drawing overlays, but theirs doesn't integrate with flycheck.)

Thanks, I see now that my email was clumsily phrased. Trying to express polite frustration ended up sounding like I was blaming you for the breakage :/ Thanks a lot for the hard work!

cpitclaudel commented 6 years ago

I merged; thanks again! (let's make sure we get a pass from the CI, too). I agree with your take on getting official buy-in, but I don't have time to dedicate to that atm, unfortunately :/ I'm fully supportive of the idea, though, if you feel like giving it a go ^^

drvink commented 6 years ago

Thanks, I see now that my email was clumsily phrased. Trying to express polite frustration ended up sounding like I was blaming you for the breakage :/ Thanks a lot for the hard work!

No problem--I knew what you meant. :)

I merged; thanks again! (let's make sure we get a pass from the CI, too).

I think you have to kick it--I don't have write access to this repo, so it won't let me restart a build.

drvink commented 6 years ago

@let-def In light of the discussion here, would you have any interest in merging this package into merlin itself?

cpitclaudel commented 6 years ago

It seems Merlin itself is having trouble building, actually (https://travis-ci.org/flycheck/flycheck-ocaml/jobs/259052793)

drvink commented 6 years ago

Ah, I see what the problem is (and why I thought Travis wasn't respecting the updated .travis.yml). Made #8; let's wait for it to pass.

let-def commented 6 years ago

I would be ok to merge this into merlin itself.

drvink commented 6 years ago

Great. Can you think of anything that would need adding besides a customization to toggle between merlin's built-in error handling or using Flycheck?

let-def commented 6 years ago

No. The truth is I am not an emacs user, so ahem, I try to develop the emacs mode but I am not really familiar with the emacs ecosystem.

juergenhoetzel commented 6 years ago

I would be ok to merge this into merlin itself.

I second that to prevent errors like this in the future. :+1:

IMO the best solution would be to add flycheck-ocaml.el to the merlin repo but provide 2 different packages on melpa (like swiper is split into counsel, swiper and ivy).

drvink commented 6 years ago

I'd like to add this to the merlin repo as well--just haven't had time to make a PR.

What would be the purpose of providing two different packages when flycheck-ocaml already depends on merlin, though?

juergenhoetzel commented 6 years ago

Yes: flycheck-ocaml.el will depend of merlin, but not the other way around. So Emacs users of merlin don't have do install the flycheck dependency if the don't need it.

drvink commented 6 years ago

Right, I discussed that in a previous comment. We would simply do the same thing that Merlin already does with its support for auto-complete and company: include flycheck-ocaml with Merlin, but not declare Flycheck in Merlin's Package-Requires header. Anyone who installs the Merlin Emacs package, whether from ELPA or using the elisp when they install it via OPAM, will be able to use the Flycheck support as long as they've installed Flycheck.

Put another way: Merlin's support for auto-complete and company aren't distributed on ELPA as separate packages, so there's no reason flycheck-ocaml need be treated differently (unless there's something I'm totally missing or I'm misunderstanding you!).

cpitclaudel commented 6 years ago

There's one issue with merging: flycheck-ocaml is GPL. Merlin is MIT.