-
Notifications
You must be signed in to change notification settings - Fork 22
Add flycheck and flycheck-clojure. #3
Add flycheck and flycheck-clojure. #3
Conversation
|
anything I can assist with? what kind of difficulties? |
|
OK, I succeeded in doing a (package-install 'flycheck). Flycheck has a couple of dependencies, and I staged everything that changed. @benedekfazekas, since you offered to help, can you check that last commit to see if anything is optional/unnecessary/undesirable? I'll work on adding flycheck-clojure manually, as it doesn't seem to be in MELPA Stable yet. |
|
I am getting an error (using emacs 24.3) at start up when using your branch as |
|
Hm, I probably installed flycheck incorrectly. I've tried to install flycheck-clojure above. If and when we get this working, I will squash all my commits. |
|
still the same error |
|
OK. I think I'll need your help installing these correctly, then. I installed these by doing an "emacs -q", and then manually setting the package configuration. I can't figure out how to load this configuration without loading my own personal configuration. Do you know how? |
|
FWIW I put this example config as my .emacs.d directory and it opened just fine, loaded and compiled an existing project, no issues at all. I'm looking forward to exploring the many features in this setup. Question for you: I do have several customizations from my old emacs config that I want to "merge" onto this config. The very easy solution I'm thinking is to just make a my-config.el file and then at the bottom of this exiting init.el do something like: (load "my-config.el") but can you clarify the elisp function for this, and the path? I see a symbol you are using dotfiles-dir but I'm not sure where that is defined, is that an emacs system var? This way if I ever want to get the latest version of this repo (which would presumably include the latest versions of all the clojure-emacs libraries included), then all I have to do is put that single line back in there and dump my my-config.el back into .emacs.d. Unless I just did a pull, in which case I assume my my-config.el would stay in place but I might still need to add that load line back in at the bottom. |
|
well, perhaps not the most elegant but you can temporary remove your |
|
I did the equivalent of that (by exporting HOME to a more convenient directory) and did not see the error you are reporting. I am using Emacs 25 head, though, so perhaps flycheck or flycheck-clojure do not support 24.3. |
|
possible. will test. |
|
the linting would be an example of one of those things i'm not entirely sure i'd want turned on by default. i assume flycheck is responsible for those squiggly red lines that appear under code sometimes? I had something like that in my old emacs config and it was always underlining stuff that was not actually an error, usually a namespace form at the top of a file for some reason. |
|
If none of the various linter programs are installed, flycheck will have no effect. |
|
if I get it right in case you use the clojure depedency of squiggly clojure that pulls in kibit and eastwood: https://github.com/clojure-emacs/squiggly-clojure#dependencies-in-clojure |
Yep. |
|
not 100% sure but it seems based on this if we want 24.3 support (yes please) then we need to install this pkgs with 24.3 emacs before pushing into the bundle |
|
OK. I don't plan to install 24.3 on my machine, so I welcome someone else to take over my fork and re-install the packages. |
|
I just looked into this, because it shouldn't take 3 days to build a package -- the tagged version has the |
Then don't check in the elpa packages: make the sample code install them on demand. Borrowing from the relevant part of my own config, you'd have something like: (add-to-list 'package-archives '("melpa-stable" . "http://stable.melpa.org/packages/"))
(defun require-package (package &optional min-version no-refresh)
"Install given PACKAGE, optionally requiring MIN-VERSION.
If NO-REFRESH is non-nil, the available package lists will not be
re-downloaded in order to locate PACKAGE."
(if (package-installed-p package min-version)
t
(if (or (assoc package package-archive-contents) no-refresh)
(package-install package)
(progn
(package-refresh-contents)
(require-package package min-version t)))))
(setq package-enable-at-startup nil) ; Don't initialize later as well
(package-initialize)Then you can just write (require-package 'clojure-mode)or before the code which |
|
Yeah, that's a good suggestion, @purcell. I'll make a separate pull in which I try to re-organize the packages in an intelligent way. And then I will revisit this pull when the MELPA Stable stuff is sorted out. |
|
Tag |
|
OK, great! I'll come back to this once we merge #4. |
Just lucky with the timing, I guess! |
9e39e38 to
0d2259c
Compare
|
let me test... |
|
perhaps i am missing something. but even if i modify the :plugins [[lein-environ "1.0.0"]]
:profiles {:dev {:env {:squiggly {:checkers [:eastwood]}}}) |
|
Did you |
|
@pnf the set-up is here: I think flycheck-clojure-setup should run when flycheck runs. And flycheck should be running at init. |
|
Can you confirm that there are clojure entries in |
|
basically works: probably my inexperience with squiggly/flycheck, sry. problem was project clj code was not loaded in the repl. using other thing: i had to manually enable flycheck mode on the clojure source file with |
|
i am ready to merge as soon as you answer the above (perhaps silly) questions... |
|
@pnf I can confirm that there are entries in flycheck checkers. Looks like that's all flycheck-clojure-setup does. For some reason I expected it to add clojure-mode to the list of modes that flycheck would work with. So, no, @benedekfazekas, it is not intended that you need to manually enable flycheck mode. Would something like this be inappropriate? (add-hook 'clojure-mode-hook (lambda () (flycheck-mode)) Alternatively, should something to that effect be added to upstream (squiggly-clojure)? |
|
above line does the trick: after a |
|
@mwfogleman @benedekfazekas There are some confusing order dependencies that I'm not sure how to ease. Don't do
If, by contrast, you had opened the source file before starting Cider, flycheck would not be started in that buffer, because there would be no checkers with passing predicates. Once you started Cider, you'd have to start flycheck manually. |
|
The more robust way around this fiddlyness would be to take the check for running Cider out of the predicate for the checker and put it into the checker itself, so it would run irrespective of whether Cider was connected but wouldn't actually do anything. This would arguably be wasteful. |
|
@pnf I tested the above step by step (only diff i used t.ns.repl/refresh): flycheck mode is still not on for the opened clojure source file: have to enable it manually. on your latest comment: wasteful in what sense? would it affect user experience in any way... |
|
You have to open the Clojure source file in emacs after Cider has been connected in order for flycheck to be started automatically. You do not have to have the namespace loaded yet for flycheck to be started automatically; it just won't report anything interesting until you load. The waste would be that every time you changed something in the source buffer without Cider running, checkers would run as no-ops. It shouldn't affect the user experience except by consuming slightly more CPU. I'm going to try this. |
|
that is what i did as you described step by step:
in fact i tried loading the source both before and after 2.: it did not matter as flycheck mode had not got enabled. tbh i could have kept an eye on nrepl msgs buffer (i did not) but i assume since mode was off there was nothing there worth noting. |
|
Then I don't know understand what's going on. If the Clojure checkers are in (defun flycheck-clojure-may-use-cider-checker ()
(let ((connection-buffer (nrepl-current-connection-buffer t)))
(and (bound-and-true-p cider-mode)
connection-buffer
(buffer-live-p (get-buffer connection-buffer))
(clojure-find-ns))))so one of those clauses must be false. |
|
just thinking out loud: is not it possible that flycheck is not triggered (therefore flycheck-clojure is not triggered as a result)? |
|
Ok, I made the change in the master branch, so cider connection is checked every time the checker runs, rather than in the predicate that determines if the checker should be loaded. I tested this from a fresh emacs launch by
|
|
tested with squiggly's own sample project: same problem (no flycheck mode for clojure source file). just to rule out that the problem is with the setup of the project i use for testing. |
|
tested your fix: works for both squiggly sample project and for the project i work with (flycheck mode is on and in fact i don't need to load the ns in the repl) :) i guess you need to release your fix to melpa stable so we can access it... |
|
👍 |
|
okay. let me know when solved so i can retest. then finally merge. woot woot ;) thx |
|
@pnf I replied to the corresponding issue in squiggly-clojure at clojure-emacs/squiggly-clojure#17. I think, it should be fixed in Flycheck itself, and I already pushed a corresponding branch, see flycheck/flycheck#568. Just waiting for the green light from Travis CI… |
|
@lunaryorn So, if I read |
|
@pnf Yes, absolutely. |
|
Looks like the Flycheck end of things is sorted out. |
|
For most intents and purposes, it's sorted out on the flycheck-clojure side too. There will be an insignificant performance gain once I restore the original predicate, but it's not crucial... so I won't feel guilty about not getting around to it today! Sent from my VAX-11/780
|
|
So functioning versions of flycheck and flycheck-clojure are on MELPA stable? |
|
nope. still |
|
any progress on this? anything I can help with? cc @pnf @lunaryorn @mwfogleman (sry for nagging ;) ) |
|
@hellofunk @mwfogleman can you guys doublecheck that this works as expected. I cheated a bit so I can merge it: flycheck dependency added from unstable melpa becuase: clojure-emacs/squiggly-clojure#17 (comment) |
This partially addresses #2. I am having difficulty loading the example-config, so I was unable to add the packages themselves. I will work on that.