clojure-emacs / squiggly-clojure

Flycheck checker for Clojure, using eastwood and core.typed.
GNU General Public License v3.0
205 stars 25 forks source link

Refactoring #9

Closed swsnr closed 9 years ago

swsnr commented 9 years ago

The refactoring promised in #5.

There are now three separate checkers for eastwood, kibit and typed, which are applied one after another through Flycheck's standard chaining mechanism. To disable specific checkers, just add them to flycheck-disabled-checkers.

The library does not implicitly register its syntax checkers anymore, to make require safe. Instead you now need to explicitly call flycheck-clojure-setup. Installation instructions and commentary are updated accordingly.

The error parsing is a little intricate, since typed apparently emits file names relative to the source directory of the parent project, which isn't exactly easy to resolve from the Emacs Lisp side. I presume this can be fixed on the clojure side of this project, but I don't know enough Clojure to do that.

The new code uses the flycheck-clojure prefix, since I dimly remember that someone somewhere suggested that this project be renamed to flycheck-clojure. If that's not the case, just tell me and I'll change the prefix to squiggly-clojure again.

I'll be away the next couple of days, and won't be able to respond, so please be patient.

swsnr commented 9 years ago

PS: Sorry that it took so long.

pnf commented 9 years ago

This is very educational for me. I know it's not the most important point, but had no idea that let-alist even existed. Anyway, give me a few hours (net of interruptions from children) to assimilate this.

pnf commented 9 years ago

A couple of things:

  1. I'd like some way to investigate failures by capturing stderr/stdin, but the callbacks are now nil. Is there an objection to having the option to transcribe output to *Messages*?
  2. It should be straightforward to choose which of the (currently three) checkers you want to run. Is there a preferred flycheck idiom for doing this? 2b. I am also considering having the selection made on the basis of Clojure namespace metadata, since the appropriateness of a particular checker is not known globally. (In particular, type checking an un-annotated namespace is a pointless nightmare.)
swsnr commented 9 years ago

@pnf

  1. I don't think it's needed, which is why I didn't add any.

After all, Emacs has the ability to modify itself while it's running: You can always inject debugging code into your live Emacs session.

Just visit the function you'd like to debug, add some debugging code (in this case, callbacks for the stdout/stderr handlers that print to *Messages*), and press C-M-x to re-evaluate the function definition in the current Emacs session. Voilà, you now see the output logged. If you're done, just undo the changes, press C-M-x again, and you are back to the original state.

That's what I did while refactoring your package, that's how I generally debug Flycheck itself and all of my other Emacs packages, and that's also the reason why you hardly ever see any debugging/logging code in Emacs packages: You can inject logging/debugging into any part of Emacs, any time. C-M-x is your friend.

  1. You can manually choose specific syntax checkers for a current buffer with M-x flycheck-select-checker, and you can disable syntax checkers by adding them to flycheck-disabled-checkers. For instance, M-x add-dir-local-variable RET clojure-mode RET flycheck-disabled-checkers RET (clojure-typed) creates a .dir-locals.el file that disables the clojure-cider-typed syntax checker for all Clojure files in your project. You can also M-x customize-variable RET flycheck-disabled-checkers and add clojure-cider-typed there to permanently disable this checker in your Emacs configuration.

2b. Sounds like you need a :predicate. Syntax checkers have :predicate functions which Flycheck calls to determine whether a syntax checker can be applied to the buffer. Take a look at the kibit checker: Its :predicate ensures that the checker is only used if the buffer is actually linked to a file.

You can write arbitrary predicates, that inspect the buffer contents to check whether a syntax checker is applicable. For instance, your predicate for clojure-cider-typed could use a regular expression to search for a type annotation and return nil if there is no type annotation in the current buffer. Flycheck will not use the typed checker then.

pnf commented 9 years ago

The problem here is that the underlying linting tools are not entirely production quality, so tweaking the choice of checkers and scrutinizing their behavior is going to be fairly common. Your suggestion of a :predicate for detecting the applicability of type checking is a good one, but it won't handle the all-too-common case where core.typed gets tripped up on apparently reasonable annotations. Clojurians are, unfortunately, used to tooling inadequacies, so problems like this won't unduly upset them, but I don't want to make their process more cumbersome. All this said, I don't think I'll have to change the PR very much. On the debugging-the-debugging side, I realized that Cider is already logging the everything in *nrepl-messages*, so it will suffice to add a sentence to the README pointing this out. For other customization, I'm going with Clojure namespace metadata, which will be comfortable for Clojure developers and keep the customization bound to the most pertinent level of granularity.

swsnr commented 9 years ago

@pnf Not sure whether I can follow you: Is flycheck-disabled-checkers too complicated? You can always set it with M-x set-variable RET flycheck-disabled-checkers, or by writing the corresponding Emacs Lisp code into your *scratch* buffer and pressing C-j.

I'm also not sure what “namespace metadata” is, but Emacs already has a good concept for buffer- and project-local settings, namely file and directory variables.

What am I missing?

pnf commented 9 years ago

@lunaryorn Clojure namespaces, e.g.

(ns sample-project.core
  {:linters #{kibit eastwood typed}}
  ;; ...)

The metadata could be interpreted by the Clojure wrapper or by a :predicate in Emacs. I see two advantages over Emacs local variables. First, it could potentially be used by other IDEs. Second, it's extensible to further customization I had planned. As an example of the latter, Eastwood comprises dozens of specific checks, which are generally customized via the :exclude-linters field of an options map.

magnars commented 9 years ago

Are you sure you want those littered throughout all your files, cluttering all the namespaces? I'd much rather customize it per-project.

swsnr commented 9 years ago

@pnf In addition to what @magnars said, do you also want to have Emacs-specific settings end up in your code and your VCS?

I'd rather add proper Emacs options to this library, and then recommend .dir-locals.el to set these settings privately, per project. I see no point in re-inventing a system specifically for Emacs customization, that Emacs actually already provides, in a slightly different, but non-invasive, well-tested and easy-to-understand fashion.

They'll never be understood by other IDEs anyway, because each IDE has their own approach to things, and standardization of such intricate and highly specific options never worked well (see http://editorconfig.org/ that basically no one uses).

And I doubt that your users would take this well. Emacs users generally expect to be able to set options from their init file or via local variables. I for my part would not want to use a package that forces me to set options in a very specific way directly in source code, depriving me of all the power and flexibility of standard Emacs options.

That said, speaking as the Flycheck maintainer, I won't object to either way. It's your choice, and I don't use Clojure anyway. I'd prefer, though, if extensions to Flycheck were good Emacs citizens and supported the standard means of customization that Flycheck and Emacs offer.

pnf commented 9 years ago

@lunaryorn My point is that these are not emacs-specific settings. They're Clojure-specific settings, which can ultimately be consumed by multiple IDEs and build processes. @magnars Yes, you'll be able to set options in the the project.clj.

pnf commented 9 years ago

PR merged.