-
Notifications
You must be signed in to change notification settings - Fork 16
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
Allow to use quelpa-use-package as source of truth for quelpa-cache #26
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments on individual lines. Beyond those, please write a bit about the purpose of this PR and why it's necessary, to help me understand it.
(when quelpa-use-package-as-source-of-truth | ||
(setq quelpa-persistent-cache-p nil)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This form belongs in the defcustom
's setter so it will take effect when the option is changed.
:group 'quelpa-use-package) | ||
|
||
(defcustom quelpa-use-package-as-source-of-truth nil | ||
"If non-nil, only packages installed via this will be managed by `quelpa'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be unclear what "this" refers to.
As well, it seems unclear what "managed by Quelpa" means.
(defgroup quelpa-use-package nil | ||
"Quelpa handler for `use-package'." | ||
:prefix "quelpa-use-package" | ||
:group 'use-package) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the parent group be use-package
or quelpa
?
(defvar quelpa-use-package-inhibit-loading-quelpa nil | ||
(defgroup quelpa-use-package nil | ||
"Quelpa handler for `use-package'." | ||
:prefix "quelpa-use-package" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not harmful to specify the prefix, but it's unnecessary for options specified later in the same file.
:prefix "quelpa-use-package" | ||
:group 'use-package) | ||
|
||
(defcustom quelpa-use-package-inhibit-loading-quelpa nil | ||
"If non-nil, `quelpa-use-package' will do its best to avoid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first line of a docstring should be a complete sentence. If this one's is being updated, it might as well be fixed.
:type 'boolean | ||
:group 'quelpa-use-package) | ||
|
||
(defcustom quelpa-use-package-as-source-of-truth nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this variable seems unclear, and a bit mysterious. Could it be given a more descriptive one?
Thanks, let me clarify the purpose of this PR before addressing your comments. So, if we don't install any new package with Since we have this |
This allows the
quelpa-cache
to be populated from the usage ofquelpa-use-package
only.Since we're using
quelpa-cache
as a way to indicate which packages are managed byquelpa
, this change allows those packages are correctly populated via the config files only.