-
Notifications
You must be signed in to change notification settings - Fork 347
Integrate haskell-flycheck into haskell-mode #538
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
Comments
This was already discussed somewhere else (can't remember where, currently), and basically I won't merge any of @chrisdone's code unless he agrees personally—that's just a matter of fairness, imho—, and I won't merge it, if I have to maintain it on my own. It's too complicated for that, and I don't know Haskell Mode well enough, nor do I have the time to get to know it. IOW, I'll only merge this checker, if @chrisdone agrees, and if he or someone else other than me agrees to maintain this checker as part of this repository. |
We discussed in a haskell-flycheck issue previously. We don't have to merge it into flychecker. That was just one option. The other option is going into Either way I want it to turn into something easy enough for people to get that it starts getting wider adoption. That's my whole goal here. |
@lukehoersten Oh sorry, there's apparently a misunderstanding. flycheck-haskell is not built-in. It's a Flycheck extension, which isn't subject to any policies about Flycheck itself. As a matter of fact, flycheck-haskell already depends on Haskell Mode already, albeit only for some minor stuff. I understand, that you'd like to have a haskell-process based checker published, but the fact that @chrisdone hasn't published it yet makes me think that it's probably not ready yet for a public release. Anyhow, I'm not at all opposed to merging everything in flycheck-haskell, as long as @chrisdone agrees, and someone else other than me actually maintains the checker. |
I do think it makes sense to preserve the current checker as an option (maybe even the default option) as well. I'm not so concerned with where the package goes, just as long as it's reachable form |
@lukehoersten The current checker will stay anyway, since it's part of Flycheck itself, and still has a purpose, e.g. for small haskell sources, which don't have a cabal file. Flycheck Haskell currently just parses Cabal files and sets up Flycheck accordingly. It doesn't have any checkers on its own. But again, the decision whether to make the haskell-process based checker available via package.el is not ours. |
Be patient and give him a chance to respond. To be clear: I'm not disputing that the decision to merge is up to @chrisdone. In the mean time, I don't want to confuse the discussion about how we would integrate the checkers with whether @chrisdone is OK with it. @chrisdone has mentioned earlier that integration specifics is his main concern:
So this brings me back to my last point: how will this work? We have a few options:
The focus here is on how does the user specify which checker should be used. If you're using |
@lukehoersten In my opinion it makes no chance for us to discuss anything as long as he hasn't responded. |
From our previous discussion:
|
@lukehoersten I read “concerned” first, whereas you apparently read “I'm okay” :) But I guess we can take that as agreement of @chrisdone, so please feel free (you or @chrisdone, whoever wants) to open a pull request that adds his Cabal Repl checker to this repository. If @chrisdone wants, I can give him write access, but I'd like to review the checker before merging it into this package and thus released it to a greater audience. If another way of integration is desired, e.g. moving everything into Haskell Mode or so, please tell me. I don't cling to this repo, so if you see a better place for this code, I'd be ok to move it somewhere else. |
@lunaryorn, Chris is concerned about changing the default checker but he's OK with merging the packages. Preserving the default means we need to work out the technical details about how the two checkers will interact and become enabled. Until the technical details are worked out, we will not merge anything. Unless you are ready to contribute to this technical discussion I'm afraid we're stalled. |
@lunaryorn: I know that you were in the middle of rewrite of generalized checker. How far are you with this? Currently @lukehoersten: I knew that |
I don't believe that My comment above enumerates some of the possibilities and potential tradeoffs of merging My personal preference is to merge |
I did not know that |
In hind sight NOTE: I'm not really proposing those name changes, just using them for illustrative purposes. |
@gracjan I'm not rewriting the built-in GHC syntax checker. I wonder where you got this idea from. I had the idea to use @lukehoersten We are not stalled then. I'll contribute to the technical aspects of the syntax checker implementation, with regards to how we can seamlessly switch between GHC and haskell-process based checking depending on whether a running Cabal REPL is available for the current buffer. Flycheck already provides the necessary infrastructure, and if haskell-process does, too, we shouldn't have any problems. I'd just like to ask you to open a PR with the new checker first. It's just easier to discuss technical aspects with the code at hand :) |
To be clear, I wrote haskell-flycheck as an experiment, I'm not convinced it's a good idea, as it has subtle interactions with e.g. the REPL context. Although it seems to work. Relatedly, ide-backend being open sourced changes the situation. Perhaps in a month we'll have a flycheck checker for ide-backend-client, so this discussion could be for naught. I don't know. |
Good to know, thanks for the update. |
@chrisdone Since you've already implemented it, it's worth a try imho. If it's too flaky we can always return to the simple GHC checker. Thanks to Flycheck's automatic checker selection it should be mostly transparent to the user. I'm not sure whether ide-backend changes the game currently. If I remember the announcement post aright, it doesn't yet expose compilation errors… if it did, thought, it'd be trivial to call out to |
Fair enough. If it's not disrupting existing users of Flycheck-haskell it seems fine to me.
It does do that, I just didn't do anything interesting with the output in my emacs minor mode yet. It'll just be a case of calling |
@lunaryorn Example, if interested: https://github.com/chrisdone/ide-backend-mode/blob/master/ide-backend-mode.el#L432 Here I'm just message'ing them but of course that could easily be pushed into a list via flycheck's async API. |
Does everyone generally agree that we should be using This means we now have two processes running during dev work: ghci for repl and ide-backend for everything else. This isn't necessarily bad, just clarifying? A goal of mine is to keep the number of external dependencies to a minimum in my dev environment just to keep things simple and fast. If that's the case, @chrisdone would you just prefer waiting until you've integrated |
I don't think anyone yet is using ide-backend, but it's a possible future contender as it's a high-quality implementation worked on by Well-Typed.
Actually ide-backend supports running statements (REPL stuff) and debugger stuff, although this was never exposed by the public online FP Complete IDE. The functionality is there. So really it would just be "the backend" for everything, ideally speaking. Similar to the
That's what I'm thinking. Here is the client: https://github.com/chrisdone/ide-backend-client and here is the mode: https://github.com/chrisdone/ide-backend-mode but it's pre-alpha. I don't have an ETA but it's less than an hour's work to get the flycheck checker defined. I don't know company-mode as a library, but it'd probably be an hour to integrate autocomplete.el, so assuming the diff between company-mode and autocomplete.el is small then it should also be easy. The instigator for me will be when I get some spare time I'll start trying to use ide-backend-mode for my real work projects and at that time it'll be a forcing function for fleshing out the rest. I'll probably move both projects under the |
Makes sense. One of the things I liked about using ghci (or ghc itself) as the backend for flycheck and code nav is that it's kept up to date with ghc by core devs. If FPco decides to move on from ide-backend (hypothetically) then we stop getting support for new lang extensions etc. What does everyone think about this? GHC and GHCi move in lock-step. |
Technically the code nav and span type info come from ghci-ng which is maintained by me (which is why it doesn't support GHC 7.10 yet). GHCi proper is more limited, and harder to contribute to. If FP Complete don't upgrade ide-backend to support 7.12 then someone (e.g. me) will. |
Conceivably though if you got your ghci-ng patches into ghci proper it would be carried along with future work? |
It is conceivable. |
@lukehoersten I'm afraid, though, that you two lost me. I know nothing about ide-backend beyond what was said in the blog post, and certainly not enough to make a decision about what we should use for Flycheck. That said, I'll happily merge any checker which you see fit for this repo and keep maintaining as good as I can, and I'll give you any help that you need when it comes to the technical aspects of writing a specific checker for Flycheck. But I'm afraid I can't contribute anything reasonable to this discussion anymore :) |
Since @chrisdone is the primary developer of both |
@lukehoersten So we'll let this matter rest until ide-backend-mode is ready? |
I think so, yes. We'll see how far Chris gets with the implementation.
|
haskell-flycheck it's @chrisdone's flycheck implementation that uses a
haskell-process
as the backend.Flycheck comes with a built-in flycheck-haskell that uses GHC. The Flycheck author, @lunaryorn, has the policy of not having Flycheck depend on external modes as it's untenable. Since
haskell-flycheck
is dependent onhaskell-process
withinhaskell-mode
anyway, I think it makes sense to include @chrisdone'shaskell-flycheck
in this mode.I'd be open to making the code changes myself and submitting a PR but I'm not sure the best approach to integrating something like this, especially since it's conditional on the availability of Flycheck.
The text was updated successfully, but these errors were encountered: