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

New racket checker #799

Closed
cpitclaudel opened this Issue Nov 19, 2015 · 17 comments

Comments

Projects
None yet
4 participants
@cpitclaudel
Member

cpitclaudel commented Nov 19, 2015

Re-routing the discussion from #786 for ease of tracking and inviting @soegaard and @takikawa.

To summarize:

Thanks for the help!

@greghendershott

This comment has been minimized.

greghendershott commented Nov 19, 2015

Although I only read the discussion twice quickly:

From this comment it sounds like @lunaryorn would be comfortable with replacing racket -f with raco expand?

If so that's simply:

  1. Revert commit 173728f (as well as the two commits for e.g. doc changes)
  2. Change the :command ("racket" "-f" source-inplace) to :command ("raco" "expand" source-inplace).

I think??


Would that be sandboxed? I don't know. But if it isn't:

  1. That didn't sound must-have to @lunaryorn (and FWIW I agree with his reasoning).
  2. Even if would be nice-to-have, it seems the logical and effective place to add that would be in the implementation of raco expand itself?

Having said all that: If the discussion somehow leads to needing to deliver some Racket code, I'd be happy to roll that into racket-mode.


p.s. If languages with "unsandboxed" macros are a problem, does that mean Rust needs to be removed, too?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Nov 19, 2015

@greghendershott Indeed, I think we'll revert that commit and move to raco in core Flycheck. But it would be very nice for racket-mode to bundle the sanboxed version; what do you think?

I think @lunaryorn's policy is reasonable, and I'll abide by it :) So rust & al. can stay :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Nov 20, 2015

I don't mind whether it's sandboxed or not. If the compiler runs it Flycheck can run it, too, imho ☺️

I just don't want to run all code. That's too unsafe for me, because normal code is very likely to have side-effects, whereas code that is intended to run at compilation time (e.g. macros, etc.) usually has not.

In other words, I'm fine with raco expand. Is raco part of a standard Racket installation?

@greghendershott

This comment has been minimized.

greghendershott commented Nov 20, 2015

@lunaryorn Yes, raco is included in both the full and minimal distributions of Racket.

p.s. You are one of my Emacs heroes; your blog is fantastic.

@greghendershott

This comment has been minimized.

greghendershott commented Nov 20, 2015

@cpitclaudel wrote:

I think we'll revert that commit and move to raco in core Flycheck. But it would be very nice for racket-mode to bundle the sanboxed version; what do you think?

I think both the cost and benefit aren't yet clear to me.

On the "cost" side, beyond adding something like this script as a file people get with racket-mode via MELPA ... I'm not sure what's involved. How does it get hooked up and used -- and by who, when?

On the benefit side, it seems small/theoretical to me. But that's only my opinion and I'm only one user.

Happy to discuss more, either here or in a PR/issue over on the racket-mode repo.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Nov 20, 2015

@greghendershott Thanks, that's great. Also thanks for your kind words, I'm flattered 😊

@greghendershott @cpitclaudel A sandboxed syntax checker is out of scope for Flycheck, because it introduces a dependency on a Racket script. Whether you'd like to add it to Racket Mode itself is your choice, but please discuss that over at Racket Mode, because it doesn't really affect Flycheck.

Please let's focus on a new Racket checker for Flycheck based on raco in this issue ☺️

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jan 3, 2016

Has anyone tried to implement a syntax checker based on raco expand yet?

@lunaryorn lunaryorn removed S-ready labels Jan 7, 2016

@wcummings

This comment has been minimized.

wcummings commented Feb 8, 2016

@lunaryorn Kinda hacky but I've been futzing around w/ it and this seems to work

(flycheck-define-checker racket
  "Racket syntax checker"
  :command ("raco" "expand" source-original)
  :error-patterns
  ((error line-start (file-name) ":" line ":" column ":" (message) line-end))
  :modes racket-mode)

(add-to-list 'flycheck-checkers 'racket)

Maybe someone who knows a little more than I do can suggest how to improve on this?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 10, 2016

Exciting. Do errors columns start at one or zero? I think you may need to add

:error-filter
(lambda (errors)
  (flycheck-sanitize-errors (flycheck-increment-error-columns errors)))

Btw @lunaryorn, should we apply -sanitize-errors unconditionally, in addition to other filters? Currently we only use it as a fallback if there's no other filter.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Feb 10, 2016

@cpitclaudel -sanitize-errors only makes safe changes; there's no harm in always applying it.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 10, 2016

@lunaryorn Indeed, so maybe we should just always apply it? This would simplify a number of checker definition from

:error-filter
(lambda (errors)
  (flycheck-sanitize-errors (flycheck-increment-error-columns errors)))

to

:error-filter #'flycheck-increment-error-columns
@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Feb 10, 2016

@cpitclaudel I would rather not take away the chance to inhibit it for individual checkers, or to change the order in which filters are applied.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 10, 2016

Got it.

@wcummings

This comment has been minimized.

wcummings commented Feb 10, 2016

@cpitclaudel rows start at 1, columns at 0.

https://gist.github.com/wcummings/b27b8ffa97d3724e84cc

My nick is wcummings on freenode if people prefer to hash things out on IRC :)

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 10, 2016

@wcummings I'm pretty busy these days, so IRC won't work very well. If columns start at 0, then the change is good. Do you think you could turn this into a pull request? You could draw inspiration from 173728f

@wcummings

This comment has been minimized.

wcummings commented Feb 10, 2016

will do 👍

@wcummings

This comment has been minimized.

wcummings commented Feb 11, 2016

lunaryorn added a commit that referenced this issue Feb 20, 2016

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

lunaryorn added a commit that referenced this issue Feb 20, 2016

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

lunaryorn added a commit that referenced this issue Feb 22, 2016

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

@lunaryorn lunaryorn closed this in d4c302c Feb 23, 2016

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