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

Pass relevant settings to checkdoc subprocess #937

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@cpitclaudel
Member

cpitclaudel commented Apr 2, 2016

‘flycheck-emacs-lisp-checkdoc-variables’ contains all variables that are
passed to the inferior Emacs process running checkdoc (but only if they
are already bound in the parent process).

The main use of this is allowing for file- and directory-local checkdoc
settings.

Closes GH-741.

Edit: Connects to #741.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 2, 2016

Quick note: I added (hack-local-variables) to the test setup. An alternative would be let-binding a few variables around the call to flycheck-should-syntax-check in the test that I added, but as written the test is more self-contained, and should better reflect the use that people will make of that feature.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 3, 2016

@cpitclaudel Thank you very much. I'll take a look.

flycheck.el Outdated
checkdoc-force-docstrings-flag
checkdoc-package-keywords-flag
checkdoc-spellcheck-documentation-flag
checkdoc-verb-check-experimental-flag)

This comment has been minimized.

@lunaryorn

lunaryorn Apr 3, 2016

Contributor

Could we discover all this options automatically? Perhaps by finding all options in the checkdoc custom group?

This comment has been minimized.

@cpitclaudel

cpitclaudel Apr 4, 2016

Member

Sure; I'll push the relevant code soon.

This comment has been minimized.

@cpitclaudel

cpitclaudel Apr 4, 2016

Member

Actually, I'm not sure that will work :/ It is quite possible for checkdoc to not be loaded but for a given checkdoc variable to be set, and in that case we still want to propagate the variable, right?
I don't think we can find the list of checkdoc variables without loading the package. Do we want to do that in the parent process?

This comment has been minimized.

@lunaryorn

lunaryorn Apr 4, 2016

Contributor

@cpitclaudel Oh yes, I didn't think of this.

However, I think we should try to make sure that we don't miss a variable here. Do you think that we can have a buttercup spec for this?

This comment has been minimized.

@cpitclaudel

cpitclaudel Apr 4, 2016

Member

Done :)

(flycheck-ert-def-checker-test (emacs-lisp-checkdoc) emacs-lisp
inherits-checkdoc-variables
(flycheck-ert-should-syntax-check
"language/emacs-lisp/local-checkdoc-variables.el" 'emacs-lisp-mode))

This comment has been minimized.

@lunaryorn

lunaryorn Apr 3, 2016

Contributor

Did you perhaps forget to include this file? I can't see it in this PR.

This comment has been minimized.

@cpitclaudel

cpitclaudel Apr 4, 2016

Member

Woops. That's right! Fixed.

@cpitclaudel cpitclaudel force-pushed the 741-checkdoc-inherit-configuration branch 2 times, most recently from b990842 to efa8aba Apr 4, 2016

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 4, 2016

Alright, pushed an updated version; thanks for the review!

@cpitclaudel cpitclaudel force-pushed the 741-checkdoc-inherit-configuration branch from efa8aba to e2936f2 Apr 4, 2016

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 4, 2016

I've added a buttercup spec checking that the list is up-to-date.

@cpitclaudel cpitclaudel force-pushed the 741-checkdoc-inherit-configuration branch 2 times, most recently from a9c581c to a86b814 Apr 4, 2016

(checkdoc-custom-vars
(seq-map #'car checkdoc-custom-pairs))
(checkdoc-relevant-vars
(cl-set-difference checkdoc-custom-vars tel--excluded-checkdoc-vars)))

This comment has been minimized.

@lunaryorn

lunaryorn Apr 4, 2016

Contributor

Please use seq-difference instead of cl-set-difference here. Just my personal preference for seq over cl-lib ☺️

This comment has been minimized.

@cpitclaudel

cpitclaudel Apr 4, 2016

Member

Ooh, I didn't realize that was in seq as well.

(seq-map #'car checkdoc-custom-pairs))
(checkdoc-relevant-vars
(cl-set-difference checkdoc-custom-vars tel--excluded-checkdoc-vars)))
;; Test for subset, since all variables aren't defined in all Emacsen

This comment has been minimized.

@lunaryorn

lunaryorn Apr 4, 2016

Contributor

I'd prefer if we could test for equality and instead limit the test to a particular Emacs version, i.e. with (assume (version<= "25" emacs-version)). That's safer, imho.

This comment has been minimized.

@cpitclaudel

cpitclaudel Apr 4, 2016

Member

Got it. Let's do it for 25 though, since Emacs 25 is the one that has more checkdoc variables.

(require 'flycheck-buttercup)
(defconst tel--excluded-checkdoc-vars

This comment has been minimized.

@lunaryorn

lunaryorn Apr 4, 2016

Contributor

I don't think that we need a special prefix for each test file. My other tests just use flycheck/, i.e. a trailing slash instead of dash to avoid conflicts with Flycheck itself. That should be sufficient, and it's easier to read imho. It took me a while to understand what tel actually stands for 😊

This comment has been minimized.

@cpitclaudel

cpitclaudel Apr 4, 2016

Member

Great, flycheck/ is a good choice.

@cpitclaudel cpitclaudel force-pushed the 741-checkdoc-inherit-configuration branch from a86b814 to a106679 Apr 4, 2016

Pass relevant settings to checkdoc subprocess
‘flycheck-emacs-lisp-checkdoc-variables’ contains all variables that are
passed to the inferior Emacs process running checkdoc (but only if they
are already bound in the parent process). This list is kept explicitly,
because generating it requires browsing the checkdoc customization
group, which is only populated after loading checkdoc. A new test
ensures that the list is up-to-date.

The main use of this is allowing for file- and directory-local checkdoc
settings.

Closes GH-741.

@cpitclaudel cpitclaudel force-pushed the 741-checkdoc-inherit-configuration branch from a106679 to b7c03b2 Apr 4, 2016

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 4, 2016

Ok, build fixed :)

@lunaryorn lunaryorn closed this in ead2b61 Apr 30, 2016

lunaryorn added a commit that referenced this pull request Apr 30, 2016

Skip checkdoc settings test on 24.3
The tested setting doesn’t exist in that version.

See GH-937

lunaryorn added a commit that referenced this pull request Apr 30, 2016

Skip checkdoc settings test on 24.3
The tested setting doesn’t exist in that version.

See GH-937

@lunaryorn lunaryorn deleted the 741-checkdoc-inherit-configuration branch Apr 30, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 30, 2016

@cpitclaudel Merged, with some little refinements. Specifically I noticed that we need to bind enable-local-variables to :safe around (hack-local-variables) to make sure that Emacs doesn't prompt for configuration when a resource file contains unsafe variables. That'd make the test suite stuck.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 30, 2016

Awesome; nice catch!

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