Skip to content
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

Racket checker w/ raco expand #873

Closed
wants to merge 7 commits into from

Conversation

@wcummings
Copy link

commented Feb 11, 2016

Can't actually get the tests to run, output in flycheck-list-errors matches up, though.

Edit (@lunaryorn): Connects to #799

William Cummings added some commits Feb 11, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

@wcummings I'll review the change and run the tests. Thank you very much for this contribution, that's awesome 👍

flycheck.el Outdated
"Racket syntax checker.

See URL `https://racket-lang.org/'."
:command ("raco" "expand" source-original)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Feb 11, 2016

Contributor

Why do we need source-original?

This comment has been minimized.

Copy link
@wcummings

wcummings Feb 11, 2016

Author

So it resolves modules correctly and tells if you mess up a require or something. I figured it was fine since raco expand (afaik) shouldn't modify the input source. Is there a nicer way to do this?

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Feb 11, 2016

Contributor

Wouldn't source-inplace work as well?

This comment has been minimized.

Copy link
@wcummings

wcummings Feb 11, 2016

Author

You're right, it would 💯

William Cummings
@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2016

@wcummings How to I install raco expand?

I installed racket with brew install racket, and I do have a raco command, but:

$ raco expand
/usr/local/bin/raco: Unrecognized command: expand

Usage: raco <command> <option> ... <arg> ...

Frequently used commands:
  setup   install and build libraries and documentation
  pkg     manage packages

A command can be specified by an unambiguous prefix.
See `raco help' for a complete list of commands.
See `raco help <command>' for help on a command.

Edit: That's the list of commands:

$ raco help
Usage: raco <command> <option> ... <arg> ...

Frequently used commands:
  setup   install and build libraries and documentation
  pkg     manage packages

All available commands:
  link    manage library-collection directories
  pkg     manage packages
  setup   install and build libraries and documentation

A command can be specified by an unambiguous prefix.
See `raco help <command>' for help on a command.

/cc @greghendershott

@greghendershott

This comment has been minimized.

Copy link

commented Feb 14, 2016

How to I install raco expand?

I installed racket with brew install racket, and I do have a raco command, but:

Although I appreciate why homebrew chose a recipe using Minimal Racket, it's unfortunate for first-time users. Not only are none of Racket's wonderful batteries included, neither is a bulb or lens. :) I'm fairly familiar with Racket and I didn't realize something like raco expand isn't included.

You could raco pkg install drracket as I suggest here for racket-mode users. That's "heavier" than you need (maybe someone else can determine the exact package(s) for raco expand and chime in). Anyway you can raco pkg remove drracket later.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2016

@greghendershott So do I understand correctly that it's possible and also likely (at least on OS X) to have raco but not raco expand?

In this case we'll need a :predicate that checks whether raco expand is present before we can safely add this syntax checker to Flycheck.

@wcummings Would you mind to update your pull request accordingly? Take a look at the go-vet checker to see how we do that in another similar case.

@greghendershott

This comment has been minimized.

Copy link

commented Feb 15, 2016

So do I understand correctly that it's possible and also likely (at least on OS X) to have raco but not raco expand?

In some other thread here I'd said I believed the raco expand command would be included even in Minimal Racket. Your recent comment made me think I'm wrong. But I'm not sure. I only ever use the full distribution of Racket. Although I won't have time today to experiment with Minimal Racket on OS X, I'll ping #racket with a link so hopefully someone who knows for sure can chime in:

  • Is raco expand in MR?
  • If not, what's the lightest raco pkg install to get it? (Because that would be a good tip for Flycheck README or error message or whatever is appropriate.)
@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2016

@greghendershott As I've said, brew install racket doesn't include raco expand. I haven't looked any further, as I don't use racket.

Generally we don't document requirements of individual syntax checkers in the README. We list them in the manual, and use our "verification" feature to provide guidelines to the user. That's why the checker needs to have a proper :predicate.

@greghendershott

This comment has been minimized.

Copy link

commented Feb 15, 2016

I don't doubt what you're seeing. I doubt my own knowledge; I don't want to mislead you. I asked on #racket, hopefully someone will advise here soon.

@samth

This comment has been minimized.

Copy link

commented Feb 15, 2016

The raco expand command is part of the compiler-lib package.

@greghendershott

This comment has been minimized.

Copy link

commented Feb 15, 2016

Thanks @samth !

So IIUC:

  1. You'll need a :predicate.
  2. raco pkg install compiler-lib will install an expand command for raco. (People using the full download from racket-lang.org will already have this. )
@wcummings

This comment has been minimized.

Copy link
Author

commented Feb 15, 2016

@lunaryorn will do 👍

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

@wcummings Thank you very much. Please ping us when you have updated the pull request. Github doesn't send notifications for pull requests; hence we might miss your update otherwise ☺️

William Cummings added some commits Feb 17, 2016

@wcummings

This comment has been minimized.

Copy link
Author

commented Feb 17, 2016

@lunaryorn I added a predicate, works alright, but it does seem noticeably more sluggish to me with the predicate, does it get checked on every call?

William Cummings
@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2016

@wcummings Yes, on every call, necessarily—Emacs sessions can run for days, weeks, even months, so it's somewhat likely that the environment changes underneath a running Emacs session. We prefer to err on the side of caution to avoid confusion error messages.

However, an additional process call by itself shouldn't be sluggish. It's probably raco pkg show that's slow. How about this predicate instead which just checks whether expand is a valid command?

(lambda ()
  (let ((raco (flycheck-checker-executable 'racket)))
    (with-temp-buffer
      (call-process raco nil t nil "expand")
      (goto-char (point-min))
      (not (looking-at-p (rx bol (1+ not-newline) "Unrecognized command: expand" eol))))))

I couldn't install compiler-libraco pkg install failed to connect to mirror.racket-lang.org—so I couldn't test whether it works in the "good" case. Would you mind to try it and check whether it works better?

William Cummings
@wcummings

This comment has been minimized.

Copy link
Author

commented Feb 17, 2016

@lunaryorn much better 👍

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2016

@wcummings Does it work well?

@wcummings

This comment has been minimized.

Copy link
Author

commented Feb 17, 2016

@lunaryorn yeah, snappier than before, at least to my eyes.

lunaryorn added a commit that referenced this pull request Feb 20, 2016

Add new racket checker
Closes GH-799 and closes GH-873
@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2016

@wcummings Thank you very much. 👍 I've cherry-picked your patch onto a branch :)

I noticed that you made your commits with an email that is not associated with your Github account. Was that by accident? Would you like me to amend the cherry-picked commits to use a different email address before finally merging to master?

lunaryorn added a commit that referenced this pull request Feb 20, 2016

Add new racket checker
Closes GH-799 and closes GH-873
@wcummings

This comment has been minimized.

Copy link
Author

commented Feb 20, 2016

@lunaryorn will@wpc.io, thanks! :)

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

@wcummings Wonderful work! Thanks :)

lunaryorn added a commit that referenced this pull request Feb 22, 2016

Add new racket checker
Closes GH-799 and closes GH-873
@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2016

@wcummings I've picked that address now, but it doesn't seem to be associated to your account either, albeit being shown in your profile. I would recommend you to check https://github.com/settings/emails and add this address (and probably the other one you used first as well) there to properly claim authorship of your commits on Github :)

@lunaryorn lunaryorn closed this in d4c302c Feb 23, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

@wcummings I applied your change to master, and with the next MELPA build Flycheck will have Racket support again. Thank you very much for your contribution 👍 Great work 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.