Skip to content

Commit

Permalink
Introduce consult-config for very flexible configuration
Browse files Browse the repository at this point in the history 
Remove many consult-preview-* customizable variables.
  • Loading branch information
@minad
minad committed Jan 5, 2021
1 parent 3042209 commit 36c9f20
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 145 deletions.
33 changes: 18 additions & 15 deletions README.org
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ Most provided commands follow the naming scheme =consult-<thing>=.
corresponding position. It is possible to jump back and forth between
the minibuffer and the buffer to perform recursive editing while the
search is ongoing. Previews must be explicitly enabled via
=consult-preview-mode=. Furthermore for each command, a customizable
variable is offered to selectively enable/disable preview.
=consult-preview-mode=. Furthermore each command can be configured
individually to selectively enable/disable preview.

** Narrowing to subsets

Expand Down Expand Up @@ -354,7 +354,9 @@ components.
| consult-async-input-throttle | 0.5 | Input throttle for asynchronous commands |
| consult-async-min-input | 3 | Minimum numbers of letters needed for async process |
| consult-async-refresh-delay | 0.25 | Refresh delay for asynchronous commands |
| consult-config | nil | Invididual command option configuration |
| consult-find-command | '(...) | Command line arguments for find |
| consult-fontify-limit | 1048576 | Buffers larger than this limit are not fontified |
| consult-git-grep-command | '(...) | Command line arguments for git-grep |
| consult-goto-line-numbers | t | Show line numbers for =consult-goto-line= |
| consult-grep-command | '(...) | Command line arguments for grep |
Expand All @@ -363,29 +365,30 @@ components.
| consult-line-numbers-widen | t | Show absolute line numbers when narrowing is active. |
| consult-line-point-placement | 'match-beginning | Placement of the point used by =consult-line= |
| consult-locate-command | '(...) | Command line arguments for locate |
| consult-mode-command-filter | "-mode$\ | --" |
| consult-mode-command-filter | ... | Filter for =consult-mode-command= |
| consult-mode-histories | ... | Mode-specific history variables |
| consult-narrow-key | nil | Narrowing prefix key during completion |
| consult-preview-buffer | t | Enable buffer preview during selection |
| consult-preview-error | t | Enable error preview during selection |
| consult-preview-flycheck | t | Enable flycheck error preview during selection |
| consult-preview-flymake | t | Enable flymake diagnostic preview during selection |
| consult-preview-global-mark | t | Enable global mark preview during selection |
| consult-preview-grep | t | Enable grep preview during selection |
| consult-preview-line | t | Enable line preview during selection |
| consult-preview-mark | t | Enable mark preview during selection |
| consult-preview-max-count | 10 | Maximum number of files to keep open during preview |
| consult-preview-max-size | 102400 | Size limit for previewed files |
| consult-preview-outline | t | Enable outline preview during selection |
| consult-preview-theme | t | Enable theme preview during selection |
| consult-preview-yank | t | Enable yank preview during selection |
| consult-preview-max-size | 10485760 | Size limit for previewed files |
| consult-project-root-function | nil | Function which returns current project root |
| consult-ripgrep-command | '(...) | Command line arguments for ripgrep |
| consult-themes | nil | List of themes to be presented for selection |
| consult-view-list-function | nil | Function which returns a list of view names as strings |
| consult-view-open-function | nil | Function to open a view by name |
| consult-widen-key | nil | Widening key during completion |

Commands allow flexible, individual customization by setting the
=consult-config= list. You can set override any option passed to
=consult--read=, for example =:preview=, =:initial=, =:default=, =:history= or
=:sort=. Note that `consult--read` is part of the internal API as of now.
Options could be removed, replaced or renamed at any time.

#+BEGIN_SRC emacs-lisp
;; Disable preview for `consult-buffer' and `consult-theme'.
(setq consult-config '((consult-theme :preview nil)
(consult-buffer :preview nil)))
#+END_SRC

** Related packages

*NOTE: If you are using Selectrum, you MUST install the packages selectrum, consult and consult-selectrum!*
Expand Down
155 changes: 63 additions & 92 deletions consult.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,12 @@
:group 'convenience
:prefix "consult-")

;;;; General customization
;;;; Customization

;;;###autoload
(define-minor-mode consult-preview-mode
"Enable preview for consult commands."
:global t)

(defcustom consult-narrow-key nil
"Prefix key for narrowing during completion."
Expand Down Expand Up @@ -196,52 +201,6 @@ You may want to add a function which pulses the current line, e.g.,
"Command line arguments for locate."
:type '(repeat string))

;;;; Preview customization

(defgroup consult-preview nil
"Preview settings of consult."
:group 'consult)

(defcustom consult-preview-buffer t
"Enable buffer preview during selection."
:type 'boolean)

(defcustom consult-preview-imenu t
"Enable imenu item preview during selection."
:type 'boolean)

(defcustom consult-preview-theme t
"Enable theme preview during selection."
:type 'boolean)

(defcustom consult-preview-yank t
"Enable yank preview during selection."
:type 'boolean)

(defcustom consult-preview-global-mark t
"Enable global mark preview during selection."
:type 'boolean)

(defcustom consult-preview-mark t
"Enable mark preview during selection."
:type 'boolean)

(defcustom consult-preview-line t
"Enable line preview during selection."
:type 'boolean)

(defcustom consult-preview-error t
"Enable error preview during selection."
:type 'boolean)

(defcustom consult-preview-outline t
"Enable outline preview during selection."
:type 'boolean)

(defcustom consult-preview-grep t
"Enable grep preview during selection."
:type 'boolean)

(defcustom consult-preview-max-size 10485760
"Files larger than this limit are not previewed."
:type 'integer)
Expand All @@ -250,10 +209,9 @@ You may want to add a function which pulses the current line, e.g.,
"Number of files to keep open at once during preview."
:type 'integer)

;;;###autoload
(define-minor-mode consult-preview-mode
"Enable preview for consult commands."
:global t)
(defcustom consult-config nil
"Command configuration alists."
:type '(list (cons symbol plist)))

;;;; Faces

Expand Down Expand Up @@ -835,8 +793,13 @@ ARGS is the open function argument for BODY."
(sort t) (default-top t) (lookup (lambda (_input _cands x) x)))
"Simplified completing read function.
Arguments:
PROMPT is the string to prompt with.
CANDIDATES is the candidate list or alist.
Options:
PREDICATE is a filter function for the candidates.
REQUIRE-MATCH equals t means that an exact match is required.
HISTORY is the symbol of the history variable.
Expand Down Expand Up @@ -891,6 +854,18 @@ NARROW is an alist of narrowing prefix strings and description."
((pred symbolp)))
(car result)))))

(defun consult--read-config (args)
"Advice for `consult--read' which adjusts ARGS, merging the command configuration."
(when-let (config (alist-get this-command consult-config))
(setq args (copy-sequence args))
(let ((options (cddr args)))
(while config
(setq options (plist-put options (car config) (cadr config))
config (cddr config)))
(setf (cddr args) options)))
args)
(advice-add #'consult--read :filter-args #'consult--read-config)

(defun consult--count-lines (pos)
"Move to position POS and return number of lines."
(let ((line 0))
Expand Down Expand Up @@ -1219,7 +1194,7 @@ This command supports candidate preview."
:lookup #'consult--line-match
:history '(:input consult--line-history)
:add-history (thing-at-point 'symbol)
:preview (and consult-preview-outline (consult--preview-position)))))
:preview (consult--preview-position))))

(defun consult--error-candidates ()
"Return alist of errors and positions."
Expand Down Expand Up @@ -1276,7 +1251,7 @@ The command supports preview of the currently selected error."
(?i . "Info"))
:history '(:input consult--error-history)
:preview
(and consult-preview-error (consult--preview-position 'consult-preview-error)))))
(consult--preview-position 'consult-preview-error))))

(defun consult--mark-candidates ()
"Return alist of lines containing markers.
Expand Down Expand Up @@ -1315,7 +1290,7 @@ The command supports preview of the currently selected marker position."
:require-match t
:lookup #'consult--lookup-cdr
:history '(:input consult--line-history)
:preview (and consult-preview-mark (consult--preview-position)))))
:preview (consult--preview-position))))

(defun consult--global-mark-candidates ()
"Return alist of lines containing markers.
Expand Down Expand Up @@ -1361,7 +1336,7 @@ The command supports preview of the currently selected marker position."
:require-match t
:lookup #'consult--lookup-cdr
:history '(:input consult--line-history)
:preview (and consult-preview-global-mark (consult--preview-position)))))
:preview (consult--preview-position))))

(defun consult--line-candidates ()
"Return alist of lines and positions."
Expand Down Expand Up @@ -1458,7 +1433,7 @@ This command obeys narrowing. Optionally INITIAL input can be provided."
:lookup #'consult--line-match
:default (car candidates)
:initial initial
:preview (and consult-preview-line (consult--preview-position))))))
:preview (consult--preview-position)))))

;;;###autoload
(defun consult-goto-line ()
Expand Down Expand Up @@ -1649,19 +1624,18 @@ If no MODES are specified, use currently active major and minor modes."
:sort nil
:category 'kill-ring
:require-match t
:preview (when consult-preview-yank
(let* ((pt (point))
;; If previous command is yank, hide previously yanked text
(mk (or (and (eq last-command 'yank) (mark t)) pt))
(ov (consult--overlay (min pt mk) (max pt mk) 'invisible t)))
(lambda (cand restore)
(if restore
(delete-overlay ov)
;; Use `add-face-text-property' on a copy of "cand in order to merge face properties
(setq cand (copy-sequence cand))
(add-face-text-property 0 (length cand) 'consult-preview-yank t cand)
;; Use the `before-string' property since the overlay might be empty.
(overlay-put ov 'before-string cand)))))))
:preview
;; If previous command is yank, hide previously yanked text
(let* ((ov) (pt (point)) (mk (or (and (eq last-command 'yank) (mark t)) pt)))
(lambda (cand restore)
(if restore
(when ov (delete-overlay ov))
(unless ov (setq ov (consult--overlay (min pt mk) (max pt mk) 'invisible t)))
;; Use `add-face-text-property' on a copy of "cand in order to merge face properties
(setq cand (copy-sequence cand))
(add-face-text-property 0 (length cand) 'consult-preview-yank t cand)
;; Use the `before-string' property since the overlay might be empty.
(overlay-put ov 'before-string cand))))))

;; Insert selected text.
;; Adapted from the Emacs yank function.
Expand Down Expand Up @@ -1908,11 +1882,10 @@ The command supports previewing the currently selected theme."
:history 'consult--theme-history
:lookup (lambda (_input _cands x)
(and x (not (string= x "default")) (intern-soft x)))
:preview (when consult-preview-theme
(lambda (cand restore)
(cond
((and restore (not cand)) (consult-theme saved-theme))
((memq cand avail-themes) (consult-theme cand)))))
:preview (lambda (cand restore)
(cond
((and restore (not cand)) (consult-theme saved-theme))
((memq cand avail-themes) (consult-theme cand))))
:default (symbol-name (or saved-theme 'default))))))
(unless (eq theme (car custom-enabled-themes))
(mapc #'disable-theme custom-enabled-themes)
Expand Down Expand Up @@ -2006,17 +1979,16 @@ Depending on the selected item OPEN-BUFFER, OPEN-FILE or OPEN-BOOKMARK will be u
;; default to creating a new buffer.
(and (not (string-blank-p cand)) (cons open-buffer cand))))
:preview
(when consult-preview-buffer
(lambda (cand restore)
(cond
(restore
(when (buffer-live-p saved-buf)
(set-buffer saved-buf)))
;; In order to avoid slowness and unnecessary complexity, we
;; only preview buffers. Loading recent files, bookmarks or
;; views can result in expensive operations.
((and (eq (car cand) open-buffer) (get-buffer (cdr cand)))
(funcall open-buffer (cdr cand) 'norecord))))))))
(lambda (cand restore)
(cond
(restore
(when (buffer-live-p saved-buf)
(set-buffer saved-buf)))
;; In order to avoid slowness and unnecessary complexity, we
;; only preview buffers. Loading recent files, bookmarks or
;; views can result in expensive operations.
((and (eq (car cand) open-buffer) (get-buffer (cdr cand)))
(funcall open-buffer (cdr cand) 'norecord)))))))
(when selected (funcall (car selected) (cdr selected)))))

;;;###autoload
Expand Down Expand Up @@ -2201,12 +2173,11 @@ this function can jump across buffers."
"Go to item: "
(or items (user-error "Imenu is empty"))
:preview
(when consult-preview-imenu
(let ((preview (consult--preview-position)))
(lambda (cand restore)
;; Only preview simple menu items which are markers,
;; in order to avoid any bad side effects.
(funcall preview (and (markerp (cdr cand)) (cdr cand)) restore))))
(let ((preview (consult--preview-position)))
(lambda (cand restore)
;; Only preview simple menu items which are markers,
;; in order to avoid any bad side effects.
(funcall preview (and (markerp (cdr cand)) (cdr cand)) restore)))
:require-match t
:narrow
(let ((narrow (cdr (seq-find (lambda (x) (derived-mode-p (car x))) consult-imenu-narrow))))
Expand Down Expand Up @@ -2308,7 +2279,7 @@ PROMPT is the prompt string."
prompt
(consult--grep-async cmd)
:lookup (consult--grep-marker open)
:preview (and consult-preview-grep (consult--preview-position))
:preview (consult--preview-position)
:initial (concat consult-async-default-split initial)
:add-history (concat consult-async-default-split (thing-at-point 'symbol))
:require-match t
Expand Down
Loading

28 comments on commit 36c9f20

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to spoil the fun something like consult-config is certainly nice but doesn't this suffer from the same problems you get with ivy that we talked about on Selectrum side? I often wrap commands or call them within another commands, last time I checked ivy introduced some inheritance mechanism to avoid this but this is much more complicated and also make configuration more complicated than needed, I think.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which problem? I am using this-command to obtain the configuration. If you wrap a command you can let-bind this-command to avoid issues.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean making it hard to reuse configuration or adjust existing configuration, for example when calling consult commands within other commands.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean making it hard to reuse configuration or adjust existing configuration, for example when wrapping consult commands within other commands.

If you wrap a command you can let-bind this-command to avoid issues. I think we win a lot of flexibility by doing that and allow configuration of options which are not exposed in other ways. Furthermore it allows me to get rid of many customization variables. I don't see a real downside of having this.

Another option we could think of is to add a consult-local-config variable which can be let-bound with a plist. This could be used in wrappers. But you can also achieve this by setting the consult-config and this-command accordingly.

However this feature is a bit of an expert feature since it relies on private (semi stable) API, but users who write wrappers can handle this, I guess.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can let-bind this-command but then you have the problem that the inner most call tells how the configuration should be. I don't think there is something wrong with this approach ivy does something similar I just don't like the problems I remember I had when fighting with ivy.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can let-bind this-command but then you have the problem that the inner most call tells how the configuration should be. I don't think there is something wrong with this approach ivy does something similar I just don't like the problems I remember I had when fighting with ivy.

That's right. I agree that it is not without problems, but from what I am seeing right now it has many upsides, in particular the additional flexibility and the code reduction of trivial boilerplate. Could you be a bit more concrete regarding the problems you had? Did you write wrappers of wrappers of wrappers? There are also problems if you overwrite options like :predicate, :narrow, :lookup or :preview. This is not a fail safe API and it is not intended to be one.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, I should add - Manipulating this-command is quite common. Many commands depend on checking this-command or last-command to adjust their behavior. This is also very brittle, even more than this I guess.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here for example, it was always hard to reuse and adapt the configuration of existing commands.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes many commands depend on this-command checks but usually they don't allow extensive configuration through this mechanism. I think if you have many options which vary the behaviour it becomes more important to be able to adjust them according to the need of the current caller.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here for example, it was always hard to reuse and adapt the configuration of existing commands.

I don't understand. I don't have a :caller argument. I am using this-command. So we retain full reusability of the code here. You have to duplicate the config however - but I think this is actually a feature, since by just wrapping a command you make it a separate command which requires a separate config.

Yes many commands depend on this-command checks but usually they don't allow extensive configuration through this mechanism. I think if you have many options which vary the behaviour it becomes more important to be able to adjust them according to the need of the current caller.

This would be the idea of adding a consult-local-config let bound variable.

(defun my-wrapper ()
  (interactive)
  (let ((consult-local-config '(:preview nil...))))
     (consult-...))

We can add such a local config variable too if you need that. But I would still keep the global configuration based on this-command.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. I don't have a :caller argument. I am using this-command. So we retain full reusability here.

This was just the issue I found, sorry if it did not match the exact use case here but with this-command you have the problem we just talked about which also comes apparent after the first wrapper you wrote around a consult command.

This would be the idea of adding a consult-local-config let bound variable.

But this still has the problem that you can't wrap my-wrapper and reuse/adapt the configuration of my-wrapper and you also can't reuse the existing config of consult... command.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there are two reusability problems here: You can reuse the config if you hard code the caller name, but this may impede reuse of the function itself.

But what do you suggest? Do you have an alternative? Since this is a new addition, the alternative would be to remove this feature again and just do nothing. Adding back the boilerplate and losing the flexibility.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only alternative I know of is the approach we sketched for Selectrum. I don't really know what conflict potentials there are because I haven't done much with consult yet if there aren't users which run into problems with this it might also be just fine to use this approach.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only alternative I know of is the approach we sketched for Selectrum. I don't really know what conflict potentials there are because I haven't done much with consult yet if there aren't users which run into problems with this it might also be just fine to use this approach.

Which alternative exactly - I got a bit lost? Basically I implemented the configuration version similar to the one I proposed for selectrum, radian-software/selectrum#244.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the local variables minibuffer-with-setup-hook dance :) I'm no suggesting you use it, this is just the only way to cleanly do this I'm currently aware of.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yes this is basically the variant with consult-local-config, I proposed above. We can add this if the need comes up. But I guess I will keep the current version based on this-command too, since it helps me to avoid all those preview customizable variables, which have gotten out of hand. If you don't use it, it won't hurt and if someone uses this way of configuration we can see if some problems come up. Since it is based on unstable API, the stability guarantees regarding this API are less strong, so I could even kick this out again if it turns out to not work well in most cases.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the variant with consult-local-config

When all settings would be contained in one variable you can't combine them easily for example when you want to enable :sort but reuse the rest. You would need to copy the settings from the old command with :sort changed. As long as there aren't that many options this is probably not an issue, the demand to adjust some settings probably won't come up as often as with ivy which defines many more UI related settings.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When all settings would be contained in one variable you can't combine them easily for example when you want to enable :sort but reuse the rest. You would need to copy the settings from the old command with :sort changed. As long as there aren't that many options this is probably not an issue, the demand to adjust some settings probably won't come up as often as with ivy which defines many more UI related settings.

Well, just use elisp to manipulate the config. Why do we have elisp after all? I certainly wouldn't copy it by hand. Or do you mean that you want some kind of inheritance mechanism? We could add an :inherit option which performs recursive lookup in the config. But this would add some complexity. Or do you mean that you want the solution you favor in selectrum with the global configuration variables?

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inheritance in the sense that you can reuse a config but override any options one by one.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is reasonably easy to do using the existing seq/list/plist functions. But if the use case comes up we can add helpers to make this easier, or add :inherit.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand, you would then query the alist you save configs in and copy the plist to adjust it for the current session?

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I like about using variables is you don't need additional context, you just set the settings you care about and the rest is reused, it is less work for the user.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for consult it might be overkill, I'm more in the context of ivy/selectrum.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand, you would then query the alist you save configs in and copy the plist to adjust it for the current session?

Something like this.

What I like about using variables is you don't need additional context, you just set the settings you care about and the rest is reused, it is less work for the user.

Yes, I know global variables are comfortable and easy to customize. But I think they have quite a high cost otherwise - Namespace pollution, problematic side effects, they introduce the potentially undesired possibility to bypass some checks. Basically every dynamically bound global variable acts as an additional argument to every function in the call chain. From the static analysis perspective this is horror. I know it is the Emacs way, but I generally avoid it if possible. And I think Emacs also came to reason a bit when they introduced lexical scoping by default...

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually don't even add buffer-local internal variables if I can avoid it and instead use closures.

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points and I agree, though for user options there is no other way, you can use a single configuration variable for each package but this would have other problems. I'm probably not as reluctant to add global state as I should be. I'm also very happy we got Lexical-scope in Emacs, that improved the situation a lot! I also think without all the macros which set/restor state the general Emacs "everything is global" approach wouldn't be feasible at all.

@minad
Copy link
Owner Author

@minad minad commented on 36c9f20 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points and I agree, though for user options there is no other way, you can use a single configuration variable for each package but this would have other problems.

I agree that it wouldn't make sense to have a single configuration variable per package. This case here is a bit different since it allows you to hook into the argument plists. Here it is a very special case and it is justified to do it like this. But this is obviously not a generally applicable solution.

I think without all the macros which set/restor state the general Emacs "everything is global" approach wouldn't be feasible at all.

I don't think it is feasible even with that. I am happy with lexical scopes. Luckily I just started writing serious elisp after that has been added. For many years I only "used" Emacs but didn't tweak it as deeply as I am doing now ;) Maybe it is the Corona situation :-P

@clemera
Copy link
Contributor

@clemera clemera commented on 36c9f20 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, hopefully it gets better soon...

Please sign in to comment.