Open zhaojiangbin opened 6 years ago
Thanks. I don't know what's going on there. It's hard to say whether it's a bug in this or in use-package
(but I would never bet against John, haha). You'll have to look at the macro-expansion of the use-package
form to see what it's doing. I think the :custom
keyword was added relatively recently, so there could be an issue with the more complex way we're initializing custom variables. But maybe we just need to change the order of some of the defcustoms.
However, when we move the regexp creation to the scan invocations, this probably won't matter anymore. So I'd suggest leaving this until that change is made.
However, when we move the regexp creation to the scan invocations, this probably won't matter anymore. So I'd suggest leaving this until that change is made.
Sounds good.
This should be "fixed" in this branch: https://github.com/alphapapa/magit-todos/pull/27
Still happening in master f7c2b2da1b0480f425db8fac332c6aaacb499848.
It appears that use-package
expands a :custom
keyword into a customize-set-varaible
call before require 'magit-todos
.
The :config
keyword, in comparison, is expanded into a progn
form after 'magit-todos is required. Using either custom-set-variables
or customize-set-variable
works here.
There are a lot of variables at play here.
magit-todos-keywords
has a custom setter. If magit-todos
is not loaded yet, then I guess its custom setter isn't loaded. And if customize-set-variable
is called before magit-todos
is loaded, I don't know exactly what happens, and I don't know what happens later when magit-todos
is loaded. I don't know if this is an issue in magit-todos
or in use-package
. Maybe use-package
needs to call custom-reevaluate-setting
for each :custom
setting after the package is loaded.
I guess I need to move the logic from the custom setter for magit-todos-keywords
into the scanner command so it's always done dynamically.
I don't know about the other custom variables.
This is what the README of 'use-package' says about :custom
(emphasis mine):
NOTE: These are only for people who wish to keep customizations with their accompanying use-package declarations. Functionally, the only benefit over using setq in a :config block is that customizations might execute code when values are assigned.
It should call customize-set-variable
after the library is loaded otherwise how could the library code run, right?
Here's what this:
(use-package magit-todos
:custom ((magit-todos-depth 25)
(magit-todos-keywords '("TODO"))))
expands to:
(progn
(defvar use-package--warning93
#'(lambda
(keyword err)
(let
((msg
(format "%s/%s: %s" 'magit-todos keyword
(error-message-string err))))
(display-warning 'use-package msg :error))))
(condition-case-unless-debug err
(progn
(customize-set-variable 'magit-todos-depth 25 "Customized with use-package magit-todos")
(customize-set-variable 'magit-todos-keywords
'("TODO")
"Customized with use-package magit-todos")
(if
(not
(require 'magit-todos nil t))
(display-warning 'use-package
(format "Cannot load %s" 'magit-todos)
:error)))
(error
(funcall use-package--warning93 :catch err))))
As you can see, it require
's after doing customize-set-variable
. The docstring for customize-set-variable
says:
If VARIABLE has a ‘custom-set’ property, that is used for setting VARIABLE, otherwise ‘set-default’ is used.
set-default
says:
set-default is a built-in function in ‘C source code’. ... Set SYMBOL’s default value to VALUE. SYMBOL and VALUE are evaluated. The default value is seen in buffers that do not have their own values for this variable.
Looking at the docstring for defcustom
, as best I can tell, this means that the magit-todo-keywords
custom-set
property's setter will not be run, because the package has not been require
d yet.
However, it seems strange to me that this would be the case, because it seems like something that should have been considered when :custom
was added to use-package
.
Indeed, putting those calls to customize-set-variable
inside with-eval-after-load
seems to fix the problem:
(progn
(defvar use-package--warning93
#'(lambda
(keyword err)
(let
((msg
(format "%s/%s: %s" 'magit-todos keyword
(error-message-string err))))
(display-warning 'use-package msg :error))))
(condition-case-unless-debug err
(progn
(with-eval-after-load 'magit-todos
(customize-set-variable 'magit-todos-depth 25 "Customized with use-package magit-todos")
(customize-set-variable 'magit-todos-keywords
'("TODO")
"Customized with use-package magit-todos"))
(if
(not
(require 'magit-todos nil t))
(display-warning 'use-package
(format "Cannot load %s" 'magit-todos)
:error)))
(error
(funcall use-package--warning93 :catch err))))
After doing that in an Emacs instance in which magit-todos
has not yet been loaded, magit-todos-keywords
is set to ("TODO")
.
So this appears to be a bug in use-package
.
Is it really necessary to have dependent variables updated in the defcustom? If possible it would be preferable if this was done on the fly when a change is detected in magit-todos-keywords (by checking the value used when the dependent variable(s) were last updated). This way customize-set-variable is not required, setq will work in an use-package :init section, which is I think what most people expect.
This way customize-set-variable is not required, setq will work in an use-package :init section, which is I think what most people expect.
These are defined with defcustom
, not defvar
, and they use functionality defined in custom.el
, so setq
is not the correct way to set their values. Sometimes it works, but users shouldn't expect it to, because there are defined ways to set defcustom
variables.
in an use-package :init section
The :init
section is definitely the wrong way to set a variable defined in a package, because :init
is evaluated before the package is loaded. :config
can be used for defvar
variables with setq
, but again, setq
shouldn't be expected to work with defcustom
s.
You are right, all the :init setq's that I've looked in my init.el are for defcustom vars, defvars I have in :config. I suppose I should fix the defcustoms to be 100% correct. It appears that defcustom side-effect code isn't very common, otherwise I think I would have had more issues.
It appears that defcustom side-effect code isn't very common, otherwise I think I would have had more issues.
I think you're right; I don't often see it myself. It's not often necessary. But it is there when you need it.
I cannot get this working at all. magit-todos
only shows TODO
, even after configuring as per above:
(use-package magit-todos
;; :ensure t
;; :load-path "~/CodeWorks/emacs/magit-todos"
:commands (magit-todos-mode)
:hook (magit-mode . magit-todos-mode)
:config
(setq magit-todos-recursive t
magit-todos-depth 100)
(custom-set-variables
'(magit-todos-keywords (list "TODO" "FIXME" "REVIEW")))
;; :custom (magit-todos-keywords (list "TODO" "FIXME"))
)
Any idea what I'm doing wrong?
@atanasj As the readme says:
Customize settings in the magit-todos group.
So try M-x customize-group
.
Thanks @alphapapa. My config has set them as follows in the magit-todos
group:
However, this is what I am getting in magit-status
:
Not sure what I am doing wrong… Any ideas?
I just tested that functionality and verified that it works properly. Whatever problem you are having is not related to the issue being tracked here.
With the following minimal setup:
Trying to run
M-x magit-status
for the first time after Emacs restarts resulted in error: "user-error: Please add some keywords". Re-eval the above snippet fixes it.In comparison, using
custom-set-variables
within:config
just works: