Add customization keywords #508

canatella · 2017-10-17T18:30:41Z

Hello,

This PR adds two new keyword: :customize and :custom-faces. These are shortcuts to cleanup calls to customize-set-variable and custom-set-faces'in the :config part. I do not use the Emacs internal save to custom.el setup because it's not easy to track in version control. I prefer to keep my package configuration in one place and these two keywords make this easier.

(use-package comint
  :customize
  (comint-buffer-maximum-size 20000 "Increase comint buffer size.")
  (comint-prompt-read-only t "Make the prompt read only."))

(use-package eruby-mode
  :custom-faces
  (eruby-standard-face ((t (:slant italic)))))

The comments are not mandatory for the :customize keyword.

I don't know if you would be willing to add these kind of keywords, so this PR is more about asking if theses features would be of interest for you to merge as I don't think this PR can be merged as is. I'm not sure about the normalizing function. They do not look like the other one because I did not really understood them, so I simply wrote ones that did the trick.

jwiegley · 2017-10-17T19:45:53Z

OK, this is a pretty brilliant idea, thank you! Have you signed copyright papers to the FSF? That's now a requirement, since we're soon to merge into Emacs core.

canatella · 2017-10-17T20:46:29Z

Nope, where should I start for the assignment ? Should I ask on emacs-devel as suggested in the Emacs manual ?

jwiegley · 2017-10-17T21:55:46Z

Send an e-mail to assign@gnu.org and they'll get you sorted out. Thanks!

basil-conto · 2017-10-18T02:14:39Z

use-package.el

+
+(defun use-package-normalize/:customize (name-symbol keyword arg)
+  "Normalize use-package customize keyword."
+  (let ((error-msg (format  "%s wants a (<symbol> <form> <optional string comment>) or list of these" name-symbol)))


Nitpick: extraneous space after format.

basil-conto · 2017-10-18T02:25:59Z

use-package.el

+    (dolist (def arg arg)
+      (unless (listp def)
+        (use-package-error error-msg))
+      (seq-let [variable value comment] def


Emacs 24 and older do not include seq.el.

Are there some other form of destructuring binding I could use ?

you could try cl-destructuring-bind?

basil-conto · 2017-10-18T02:29:31Z

use-package.el

+  "Generate use-package customize keyword code."
+  (let ((body (use-package-process-keywords name rest state)))
+    (use-package-concat
+     (seq-map (lambda (def)


Assuming seq.el is available, is there a reason to prefer seq-map over mapcar here?

When I finally have a nice sequence elisp library, I cannot use it anymore ;) I'm just using the seq-* function in my elisp because they seem more modern then the old disparate functions, but I understand the point here and will switch to mapcar.

basil-conto · 2017-10-18T02:32:25Z

use-package.el

+
+(defun use-package-normalize/:custom-faces (name-symbol keyword arg)
+  "Normalize use-package custom-faces keyword."
+  (let ((error-msg (format  "%s wants a (<symbol> <face-spec>) or list of these" name-symbol)))


Ditto re: extraneous space.

canatella · 2017-10-18T12:45:43Z

Ok, so I

replaced seq-map by mapcar,
replaced seq-let by cl-destructuring bind,
and removed the extra spaces.

I also contacted assign@gnu.org for the paper work.

basil-conto · 2017-10-18T13:24:30Z

use-package.el

+    (dolist (def arg arg)
+      (unless (listp def)
+        (use-package-error error-msg))
+      (cl-destructuring-bind (&optional variable value comment) def


cl-destructuring-bind is a macro defined in cl-macs.el, so you will need to (eval-when-compile (require 'cl-macs)). I do not, however, think this is the best destructuring option here, as it will throw a non-use-package-specific wrong-number-of-arguments error if def has more elements than it is being destructured into (this also makes the (> (length def) 3) check further down redundant).

I suggest using pcase-let instead:

(pcase-let ((`(,variable ,value ,comment) def)) ;; ... )

This has the added benefit of introducing no further dependencies (apart from the existing Emacs24+ requirement).

(eval-when-compile (require 'cl-macs))

FYI, it should rather be (eval-when-compile (require 'cl-lib)) (e.g., see https://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01003.html).

I suggest using pcase-let instead:

Hmm, I'm not sure that one deals well with a different number of arguments either:

(pcase-let BINDINGS &rest BODY) Like ‘let’ [...] The macro is expanded and optimized under the assumption that those patterns *will* match, so a mismatch may go undetected or may cause any kind of error.

I always feared that might be the case re: cl-macs so thanks for confirming it. :)

My reading of the pcase and pcase-let documentation is that a backquote-style QPAT list pattern will always match, regardless of the length of the list being matched against. In other words, I think the pcase-let docstring is warning against more obvious infractions like (pcase-let (((and (pred listp) l) "wut"))), which don't match but which pcase would happily skip.

Maybe I'm wrong about this, and maybe destructuring isn't worth the trouble for such simple validation checks.

a backquote-style QPAT list pattern will always match, regardless of the length of the list being matched against.

Well if that were true, how would you distinguish between different list lengths? Perhaps noting that `(,one ,two ,three) is a shorthand for `(,one ,two ,three . nil) might make it clearer:

(mapcar (lambda (x) (pcase x (`(,one ,two ,three . nil) (list :exactly-three-args one two three)) (`(,one ,two ,three . ,rest) (list :more-than-three-args one two three rest)) (_ (list :less-than-three-args x)))) '((1) (1 2) (1 2 3) (1 2 3 4))) ;=> ((:less-than-three-args (1)) (:less-than-three-args (1 2)) (:exactly-three-args 1 2 3) (:more-than-three-args 1 2 3 (4)))

Going with

(let ((var (car def)) (val (cadr def)) (com (caadr def)) ...)

then ?

You mean (caddr def), but otherwise seems fine.

indeed, that proves what's nice about destructuring bind ;)

@npostavs I see, thanks for clarifying.

@canatella I don't think Emacs 24 has caddr yet. Why not just use nth for all of them? That way you afford a tiny bit more destructuring clarity.

canatella · 2017-10-18T15:31:56Z

I replaced the destructuring bind with a good old let.

canatella · 2017-10-18T16:33:41Z

Indeed, nth keeps a bit of the destructuring clarity. Replaced ca*dr with nth.

canatella · 2017-10-20T07:54:01Z

Hello,

No answer from assign@gnu.org for 2 days, does it always take a bit of time, or should I try contacting them again ?

canatella · 2017-10-20T09:38:20Z

Another thing, wouldn't it be better to customize at :init rather then :config ? I have a problem here with exec-path-from-shell which needs the exec-path-from-shell-variables to be setup before calling exec-path-from-shell-initialize.

This is not working with the current code:

(use-package exec-path-from-shell
  :ensure t
  :customize
  (exec-path-from-shell-variables
   '("PATH" "MANPATH" "ANDROID_HOME" "ANDROID_NDK_HOME"))
  :config
  (when (memq window-system '(mac ns x))
    (exec-path-from-shell-initialize)))

While this is

(use-package exec-path-from-shell
  :ensure t
  :customize
  :config
  (when (memq window-system '(mac ns x))
    (let ((exec-path-from-shell-variables
           '("PATH" "MANPATH" "ANDROID_HOME" "ANDROID_NDK_HOME")))
      (exec-path-from-shell-initialize))))

works as expected.

npostavs · 2017-10-20T12:05:28Z

No answer from assign@gnu.org for 2 days, does it always take a bit of time, or should I try contacting them again ?

I think it was something like 30 days when I did it.

canatella · 2017-10-20T12:26:00Z

Well, let's wait then...

basil-conto · 2017-10-20T13:14:21Z

I did mine pretty recently - I was sent the form within a day or two and then it was confirmed a couple of weeks later.

alphapapa · 2017-10-20T16:22:40Z

I did the CA recently and got responses within 24 hours IIRC. But AFAIK there's only one person who does that job, so I guess he's busier at some times than others.

canatella · 2017-11-08T16:03:51Z

Hello, the copyright assignment procedure is done, so we can go on but I have discovered a problem in my config though.

As is, this patch sets the custom variables at :config time. It's a problem for the exec-path-from-shell package which expects the variable exec-path-from-shell-variables to be set before it is required. Is it a problem with the package, or should the variables be set at :init time instead?

jwiegley · 2017-11-08T16:27:11Z

I believe they should be set at :init time, as regular customizations are.

canatella · 2017-11-09T12:00:56Z

I've modified the keyword order so that the customizations are done just before :init. The exec-path-from-shell package is now working correctly.

dieggsy · 2017-11-09T12:07:43Z

@canatella should :customize be :custom-variables for consistency? (I would even consider :custom-set-variables and :custom-set-faces for clarity, though those are pretty wordy)

canatella · 2017-11-09T12:50:57Z

I asked myself that question too. While I agree with you on the need for keyword consistency, I think that the `:custom-face' one would be much less used and keeping keyword length reasonable is nice.

What do other reviewers here think ?

jwiegley · 2017-11-09T16:46:05Z

I prefer :custom and :custom-face.

Allows customization of variable using customize-set-variables. This makes it easier to manage customization in version control. Instead of having all the variables written in a custom.el, the variable can be customized where the rest of the package is configured.

Allows customization of faces using customize-set-faces. This makes it easier to manage customization in version control. Instead of having all the faces written in a custom.el, the faces can be customized where the rest of the package is configured.

canatella · 2017-11-10T09:53:54Z

Good compromise, I made the change.

dieggsy · 2017-11-19T13:37:42Z

@jwiegley @canatella any news on this?

jwiegley · 2017-11-20T23:42:28Z

Thank you for a great addition!

Add customization keywords GitHub-reference: jwiegley/use-package#508

basil-conto reviewed Oct 18, 2017

View reviewed changes

canatella force-pushed the add-customization-keywords branch 2 times, most recently from 1a485c8 to b1ca822 Compare October 18, 2017 12:42

basil-conto reviewed Oct 18, 2017

View reviewed changes

canatella force-pushed the add-customization-keywords branch from b1ca822 to 627bd75 Compare October 18, 2017 15:30

canatella force-pushed the add-customization-keywords branch from 627bd75 to e68f734 Compare October 18, 2017 16:31

canatella force-pushed the add-customization-keywords branch from e68f734 to c7d41f1 Compare November 9, 2017 11:49

canatella added 2 commits November 10, 2017 09:14

Add support for face customization.

fac05d0

Allows customization of faces using customize-set-faces. This makes it easier to manage customization in version control. Instead of having all the faces written in a custom.el, the faces can be customized where the rest of the package is configured.

canatella force-pushed the add-customization-keywords branch from c7d41f1 to fac05d0 Compare November 10, 2017 08:18

jwiegley merged commit 41f61a1 into jwiegley:master Nov 20, 2017

jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 24, 2022

Merge pull request from canatella/add-customization-keywords

d3d036f

Add customization keywords GitHub-reference: jwiegley/use-package#508

jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 29, 2022

Merge pull request from canatella/add-customization-keywords

9bf8264

Add customization keywords GitHub-reference: jwiegley/use-package#508

Add customization keywords #508

Add customization keywords #508

Conversation

canatella commented Oct 17, 2017

jwiegley commented Oct 17, 2017

canatella commented Oct 17, 2017

jwiegley commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canatella commented Oct 18, 2017

basil-conto Oct 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil-conto Oct 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil-conto Oct 18, 2017 • edited

Choose a reason for hiding this comment

canatella commented Oct 18, 2017

canatella commented Oct 18, 2017

canatella commented Oct 20, 2017

canatella commented Oct 20, 2017

npostavs commented Oct 20, 2017

canatella commented Oct 20, 2017

basil-conto commented Oct 20, 2017

alphapapa commented Oct 20, 2017

canatella commented Nov 8, 2017

jwiegley commented Nov 8, 2017

canatella commented Nov 9, 2017

dieggsy commented Nov 9, 2017 • edited

canatella commented Nov 9, 2017

jwiegley commented Nov 9, 2017

canatella commented Nov 10, 2017

dieggsy commented Nov 19, 2017

jwiegley commented Nov 20, 2017

basil-conto Oct 18, 2017 •

edited

basil-conto Oct 18, 2017 •

edited

basil-conto Oct 18, 2017 •

edited

dieggsy commented Nov 9, 2017 •

edited