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

Enable Racket checker when Geiser uses Racket #979

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@dkim
Contributor

dkim commented May 26, 2016

Geiser enhances scheme-mode's support for Racket as a minor mode (geiser-mode), not as a new major mode. However, we cannot just check minor-mode-alist for geiser-mode because Geiser supports other Scheme implementations as well as Racket. .rkt is the standard file extension for Racket source code.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 26, 2016

@dkim I'm not sure whether I'd like to merge this change. As far as I see it, it's a workaround for a flaw in scheme-mode/geiser-mode which doesn't expose the scheme dialect in anyway. We should not have to check file extensions to determine the language of a buffer. After all, that's what major modes are for.

@dkim

This comment has been minimized.

Contributor

dkim commented May 26, 2016

@lunaryorn Geiser appears to be implemented that way because Racket used to be PLT Scheme. So is it difficult to let FlyCheck in the official repository support Geiser unless Geiser's support for Racket turns into a major mode?

Or is it acceptable to register the Racket checker as a syntax checker for scheme-mode as well as racket-mode (i.e. :modes (racket-mode scheme-mode)) although it might annoy those that use scheme-mode with other Scheme implementations.

@dkim

This comment has been minimized.

Contributor

dkim commented May 26, 2016

I have written a new patch https://github.com/dkim/flycheck/commit/48048c877dc81dff467744ba6e910ddfb1a927a7 that does not check file extensions although it is more specific to Geiser than the version above.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented May 27, 2016

@dkim Thanks for your help and efforts :) However, I'm not sure I like the new patch either; depending on an explicitly private variable (as indicated by -- in the name) sounds like a bad idea :/

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 27, 2016

@dkim Generally the syntax checker should have :modes '(racket-mode scheme-mode) in either case because it lets Flycheck select checkers for efficiently. With some changes ahead :modes will actually become mandatory for all syntax checkers.

We do not strictly require major modes for syntax checker selection but we should not duplicate the major mode selection of Emacs or the language guessing of certain modes like Geiser Mode by dispatching on the file extension. That's too flaky and too limited.

We can certainly dispatch on variables, though. For instance we use sh-shell to dispatch to the proper shell in Sh Mode.

As such I prefer your second patch, though I share @cpitclaudel's concerns about using a private variable from Geiser Mode. I wonder if there's no public API in Geiser Mode to find out what Scheme implementation is being used in the current buffer. Would you mind to contact the Geiser Mode developers to find out? Or suggest that they add a public variable/function which we can use to get the underlying scheme implementation?

@dkim

This comment has been minimized.

Contributor

dkim commented May 27, 2016

@cpitclaudel @lunaryorn The private variable was the best that I could find. Yesterday I asked geiser-users@nongnu.org if there is a more established way of doing it. I attached to the mail a link to this PR. I hope that we will get a reply soon.

@jaor

This comment has been minimized.

jaor commented May 28, 2016

hi, i'm geiser's author. it's perfectly fine to use geiser-impl--implementation. the reason it uses -- is that it doesn't belong to the interactive, user-level interface of geiser, that's all. it's guaranteed to remain there and be stable.

@dkim

This comment has been minimized.

Contributor

dkim commented May 28, 2016

Thank you for your reply and confirmation, @jaor!

@lunaryorn @cpitclaudel I plan to add the :modes (racket-mode scheme-mode) line to the second patch, as @lunaryorn suggested, and make a new PR. Please let me know if there is any other thing that I can improve.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 29, 2016

@jaor Thank you for joining this discussion and helping us out 👍

@dkim I'm fine with using geiser-impl--implementation in the :predicate then. I'd be great if you could submit your second patch with an appropriate :modes declaration.

Please don't open a second pull request, though. Just git push --force to the geiser branch of your fork to update this pull request. That's easier for us to keep track of things ☺️

Thanks!

Enable Racket checker when Geiser uses Racket
Geiser enhances scheme-mode's support for Racket as a minor mode
(geiser-mode). However, we cannot just check minor-mode-alist for
geiser-mode because Geiser supports other Scheme implementations as well
as Racket.

Jose Antonio Ortega Ruiz, the author of Geiser, has confirmed that
checking the geiser-impl--implementation variable is a public interface
for finding out what Scheme implementation Geiser is associating the
current buffer with: "--" in its name is just for suggesting that the
variable is not for an end user's interactive use.

The :modes declaration has some overlap in its functionality with the
:predicate declaration and yet is included because it will become
mandatory after all in the near future.

@dkim dkim changed the title from Enable Racket checker for .rkt files under scheme-mode to Enable Racket checker when Geiser uses Racket May 29, 2016

@lunaryorn lunaryorn closed this in 8154c41 Jun 2, 2016

@lunaryorn lunaryorn removed the S-to review label Jun 2, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jun 2, 2016

@dkim Thanks, merged 👍

@dkim dkim deleted the dkim:geiser branch Jun 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment