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

Add emacs-lisp-check-declare checker #1286

Merged
merged 1 commit into from Aug 2, 2017

Conversation

Projects
None yet
4 participants
@darkfeline
Contributor

darkfeline commented Jul 19, 2017

No description provided.

@CLAassistant

This comment has been minimized.

CLAassistant commented Jul 19, 2017

CLA assistant check
All committers have signed the CLA.

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 19, 2017

Hm, this appears to work when I load the checker into my working Emacs, but the flycheck-test test doesn't pass, and ert is woefully opaque about why.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jul 19, 2017

The output to ERT is not the most intuitive:

((should
       (equal expected flycheck-current-errors))
      :form
      (equal
       ([cl-struct-flycheck-error #<killed buffer> emacs-lisp-check-declare "/home/travis/build/flycheck/flycheck/test/resources/language/emacs-lisp/check-declare-errors.el" 10 1 "said ‘this-function-is-not-defined’ was defined in this-file-does-not-exist.el: file not found" warning nil nil])
       nil)

Inside the :form (equal, it expects the error you declared, but you got nil instead. That means the error was either not caught by the checker, or the checker was not run at all.

I think the issue here is that your checker is never selected by the Flycheck run by ERT. emacs-lisp is the default checker, followed by emacs-lisp-checkdoc (see the :next-checker property).

You could put your checker as a :next-checker for checkdoc.

@darkfeline darkfeline force-pushed the darkfeline:check-declare branch from 1058b7c to 2e21ec5 Jul 19, 2017

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 19, 2017

Ah, got it. The tests pass now.

@cpitclaudel

Thanks, this looks pretty good. One question: shouldn't this be part of the default ELisp checker?

;;; Commentary:
;; Checkdoc uses the syntax table and comment syntax of the current buffer; if

This comment has been minimized.

@cpitclaudel

cpitclaudel Jul 19, 2017

Member

Isn't that comment a leftover from another file?

flycheck.el Outdated
@@ -7161,12 +7162,34 @@ The checker runs `checkdoc-current-buffer'."
(not (or (flycheck-autoloads-file-p)
(and (buffer-file-name)
(member (file-name-nondirectory (buffer-file-name))
'("Cask" "Carton" ".dir-locals.el")))))))
'("Cask" "Carton" ".dir-locals.el"))))))
:next-checkers (emacs-lisp-check-declare))

This comment has been minimized.

@cpitclaudel

cpitclaudel Jul 19, 2017

Member

Shouldn't this be added to the next-checkers of emacs-lisp as well?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jul 19, 2017

shouldn't this be part of the default ELisp checker?

I agree. It looks like we could just add the check-declare-form as an additional argument to the emacs-lisp checker, and save ourselves the trouble of calling emacs a third time.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jul 19, 2017

(Reminder: #1229)

@darkfeline darkfeline force-pushed the darkfeline:check-declare branch from 2e21ec5 to c4b27e4 Jul 20, 2017

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 20, 2017

Merged checker into emacs-lisp

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 20, 2017

Looks like I need to get my hands on a build of Emacs 24, please wait warmly.

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 22, 2017

Actually, it looks like I can't build Emacs 24. I believe I'm running into the bug mentioned here: http://lists.linuxfromscratch.org/pipermail/blfs-support/2016-August/078284.html

It may be a while before I can get Emacs 24. If someone else can fix this, feel free. I suspect it's something like the error filter needing to be tweaked.

@darkfeline darkfeline force-pushed the darkfeline:check-declare branch from c4b27e4 to eb947e4 Jul 25, 2017

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 25, 2017

The Emacs I used was 24.3, which flycheck doesn't support. If the fix doesn't pass, I'll probably install a newer Ubuntu in a VM and try again.

I wonder if flycheck would benefit from shipping a Vagrantfile.

@darkfeline darkfeline force-pushed the darkfeline:check-declare branch from eb947e4 to aa4af40 Jul 25, 2017

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 25, 2017

Well, that was a waste of time. As it turns out, check-declare does not print the line number on Emacs 24 and stepping through the debugger I learned that Flycheck discards errors that do not have line numbers.

I don't really feel like trying to make this work now, so I've skipped the test on Emacs 24 and documented that check-declare doesn't work on Emacs 24.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jul 25, 2017

Thanks for investigating the issue. So, currently the check-declare pass will be run by Flycheck on Emacs 24, but the errors won't be reported, is that it?

If the issue is missing line numbers in the output, you can check for errors that have a nil line and set them to 0 instead. That way, the errors will be reported on the first line, and Flycheck will not discard them.

If it's too much a hassle to test that with Emacs 24, let me know. I have an Emacs 24 install I can use to test this workaround.

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 26, 2017

Currently on Emacs 24, check-declare will emit errors correctly, but they will be filtered out by flycheck-relevant-errors/flycheck-relevant-error-p.

Adding 0 line numbers to them sounds like a good solution; I'll try to do it with error-filters.

@darkfeline darkfeline force-pushed the darkfeline:check-declare branch 2 times, most recently from 27f03ed to 1c5d410 Jul 26, 2017

@darkfeline

This comment has been minimized.

Contributor

darkfeline commented Jul 30, 2017

Any update on this?

@fmdkdd

fmdkdd approved these changes Aug 1, 2017

@darkfeline Sorry, I didn't see that you updated the PR (does Github actually notifies you of new commits, even for amends?)

That's a lovely patch, with tests. LGTM 👍

@cpitclaudel, is it good for you?

@cpitclaudel

Great work :) I left two small comments, but I'd also be fine with merging as-is.

flycheck.el Outdated
flycheck-byte-compiled-files)))))
flycheck-byte-compiled-files))
(when (and (boundp 'flycheck-emacs-lisp-check-declare)
flycheck-emacs-lisp-check-declare)

This comment has been minimized.

@cpitclaudel

cpitclaudel Aug 1, 2017

Member

Could this be written as bound-and-true-p?

This comment has been minimized.

@darkfeline

darkfeline Aug 2, 2017

Contributor

Done

(seq-do (lambda (err)
(unless (flycheck-error-line err)
(setf (flycheck-error-line err) 0)))
errors)

This comment has been minimized.

@cpitclaudel

cpitclaudel Aug 1, 2017

Member

For simple iteration like this, I tend to prefer dolist over seq-do

This comment has been minimized.

@darkfeline

darkfeline Aug 2, 2017

Contributor

All of the other filters use seq-do, so I am inclined to keep the code consistent.

EDIT: Not all, but many. The nearby filters use seq-do so I will do the same.

@darkfeline darkfeline force-pushed the darkfeline:check-declare branch from 1c5d410 to ac7b5a9 Aug 2, 2017

@cpitclaudel cpitclaudel merged commit ed08ce3 into flycheck:master Aug 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Aug 2, 2017

Brilliant work, thanks!

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Aug 2, 2017

Thank you @darkfeline for your patience, and for this nice contribution!

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