FYI: if-let, when-let, pcase-let

[alphapapa]

Hi,

This package looks very cool! FYI, you might like to use the when-let and if-let macros, e.g. here:

https://github.com/sinic/ednc/blob/b756e31e06574426bf705e755df42aa7a19d32d1/ednc.el#L312

They're in subr-x.

Also, this is a matter of style, but in case you weren't aware of the pcase-let macro in Emacs, you could write ednc--remove-old-notification-from-log-buffer like this:

(defun ednc--remove-old-notification-from-log-buffer (old)
  "Remove OLD notification from its log buffer, if it exists."
  (pcase-let* (((map logged) (ednc-notification-amendments old))
               (`(,buffer . ,position) logged))
    (when (buffer-live-p buffer)
      (with-current-buffer buffer
        (save-excursion
          (add-text-properties (goto-char position) (line-end-position)
                               '(face (:strike-through t))))))))

Note as well that I use when instead of the "one-armed if", which is more idiomatic.

[sinic]

This package looks very cool!

Thanks! It is the first Emacs package I published, so I'm particularly grateful for comments about conventions I might be unaware of.

FYI, you might like to use the when-let and if-let macros, e.g. here:

I try to require as few libraries as possible, even if they are part of Emacs. In fact, in commit d3b03aa I replaced occurrences of the string-empty-p macro with its expansion, just to avoid requiring subr-x. This aversion may be unwarranted, I admit.

Also, this is a matter of style, but in case you weren't aware of the pcase-let macro in Emacs, you could write ednc--remove-old-notification-from-log-buffer like this:

The documentation of pcase-let* states that

Each EXP should match (i.e. be of compatible structure) to its respective PATTERN; a mismatch may signal an error or may go undetected, binding variables to arbitrary values, such as nil.

The way I interpret this paragraph, one should never do something like (pcase-let* (`(,a . ,b) nil)) a), even though it happens to evaluate to nil for me with Emacs 27.1. Unfortunately one also cannot rely, by design, on the presence of any entry in the amendments slot. For logged in specific, the hook ednc-notification-presentation-functions is part of the public interface, and users that dislike EDNC's notification log might well remove the function ednc--update-log-buffer that ultimately amends that entry.

(My cl-destructuring-bind version avoids the problem by retrieving the entry separately with alist-get, where I specify a default value that can be destructured to a dotted pair.)

Note as well that I use when instead of the "one-armed if", which is more idiomatic.

This is something I changed in commit bf5ce75 after discovering instances of (when something (something-else)) in the Emacs Lisp Style Guide, but since it is otherwise silent on the issue, I defer to your judgment if you say the other way is more idiomatic.

[alphapapa]

This package looks very cool!

Thanks! It is the first Emacs package I published, so I'm particularly grateful for comments about conventions I might be unaware of.

The style guide you mentioned is a good resource. I collect more resources here: https://github.com/alphapapa/emacs-package-dev-handbook

FYI, you might like to use the when-let and if-let macros, e.g. here:

I try to require as few libraries as possible, even if they are part of Emacs. In fact, in commit d3b03aa I replaced occurrences of the string-empty-p macro with its expansion, just to avoid requiring subr-x. This aversion may be unwarranted, I admit.

Note that string-empty-p is not a macro but an inline function defined with defsubst. The byte-compiler inlines it automatically, so you needn't do it manually. Refer to the Elisp manual section on inline functions.

There's no benefit to not using code from subr-x. It's built-in to Emacs, and many packages use it, so it will likely be loaded already, anyway.

Note as well that I use when instead of the "one-armed if", which is more idiomatic.

This is something I changed in commit bf5ce75 after discovering instances of (when something (something-else)) in the Emacs Lisp Style Guide, but since it is otherwise silent on the issue, I defer to your judgment if you say the other way is more idiomatic.

Maybe you misunderstood the example in that guide. While it doesn't use the phrase "one-armed ifs", the example of what not to do is a one-armed if, and it recommends using when instead. Besides that guide, MELPA recommends against one-armed ifs as well. I don't know of anyone who suggests using one-armed ifs. Using when is much clearer about the purpose of its body code.

Also, this is a matter of style, but in case you weren't aware of the pcase-let macro in Emacs, you could write ednc--remove-old-notification-from-log-buffer like this:

The documentation of pcase-let* states that

Each EXP should match (i.e. be of compatible structure) to its respective PATTERN; a mismatch may signal an error or may go undetected, binding variables to arbitrary values, such as nil.

The way I interpret this paragraph, one should never do something like (pcase-let* (`(,a . ,b) nil)) a), even though it happens to evaluate to nil for me with Emacs 27.1. Unfortunately one also cannot rely, by design, on the presence of any entry in the amendments slot. For logged in specific, the hook ednc-notification-presentation-functions is part of the public interface, and users that dislike EDNC's notification log might well remove the function ednc--update-log-buffer that ultimately amends that entry.

(My cl-destructuring-bind version avoids the problem by retrieving the entry separately with alist-get, where I specify a default value that can be destructured to a dotted pair.)

I don't know anything about ednc-notification-presentation-functions, etc. I don't think using pcase-let* would be a problem here, but it's certainly a matter of style, and you needn't use it. Some people like pcase, some people don't. :)

[alphapapa]

One more suggestion:

https://github.com/sinic/ednc/blob/b756e31e06574426bf705e755df42aa7a19d32d1/ednc.el#L57

It's generally recommended to format an if with the THEN form on the next line. Elisp indents the THEN form so it stands out from the ELSE forms.

From your code, it looks like you're an experienced Lisper, so I think there's not much I need to tell you. :)

[sinic]

Note that string-empty-p is not a macro but an inline function defined with defsubst.

Oops, my bad. My thoughts were with the when-let and if-let macros.

There's no benefit to not using code from subr-x. It's built-in to Emacs, and many packages use it, so it will likely be loaded already, anyway.

Alright. I'm convinced, since I cannot think of any benefit either :)

Maybe you misunderstood the example in that guide. While it doesn't use the phrase "one-armed ifs", the example of what not to do is a one-armed if, and it recommends using when instead. Besides that guide, MELPA recommends against one-armed ifs as well. I don't know of anyone who suggests using one-armed ifs. Using when is much clearer about the purpose of its body code.

The example I quoted only occurs in the Source Code Layout section. Do you mean this rule in the Syntax section?

Use when instead of (if ... (progn ...).

Technically (if ... (progn ...)) is one-armed, but if that rule's intention is what you claim it is, then it is worded poorly. Why does it bring up progn in the first place? Obviously when without progn is better in that case; the interesting case is when there is no progn and both alternatives work equally well!

Do you happen to have a link for the MELPA recommandation? I will change it either way, but I'm still curious. I really did try to find a rule before submitting it.

I don't know anything about ednc-notification-presentation-functions, etc. I don't think using pcase-let* would be a problem here, but it's certainly a matter of style, and you needn't use it. Some people like pcase, some people don't. :)

The problem is that pcase-let*, according to its documentation, is free to assign arbitrary values to buffer and position in the proposed expression if logged is nil, which is the case if (ednc-notification-amendments old) happens to be empty:

(pcase-let* (((map logged) (ednc-notification-amendments old))
             (`(,buffer . ,position) logged))

[alphapapa]

The example I quoted only occurs in the Source Code Layout section. Do you mean this rule in the Syntax section?

Use when instead of (if ... (progn ...).

Yes.

Technically (if ... (progn ...)) is one-armed, but if that rule's intention is what you claim it is, then it is worded poorly. Why does it bring up progn in the first place? Obviously when without progn is better in that case; the interesting case is when there is no progn and both alternatives work equally well!

I can't speak for the author(s) of that guide. All I know is that when is more explicit about its purpose than a one-armed if, and it makes for code that's more easily understood.

Do you happen to have a link for the MELPA recommandation? I will change it either way, but I'm still curious. I really did try to find a rule before submitting it.

I don't know if it's written in MELPA's guide. If it's not, you might submit a PR about it (or maybe not, because I think the MELPA guide is intended to be more general and less about fine details like this). But the recommendation is frequently made by Steve, Chris, et al in MELPA submissions.

I don't know anything about ednc-notification-presentation-functions, etc. I don't think using pcase-let* would be a problem here, but it's certainly a matter of style, and you needn't use it. Some people like pcase, some people don't. :)

The problem is that pcase-let*, according to its documentation, is free to assign arbitrary values to buffer and position in the proposed expression if logged is nil, which is the case if (ednc-notification-amendments old) happens to be empty:

(pcase-let* (((map logged) (ednc-notification-amendments old))
             (`(,buffer . ,position) logged))

In the case that logged is nil, I can't imagine buffer and/or position ever being anything but nil. But if you're concerned about this, it's not necessary to use pcase.

[sinic]

(or maybe not, because I think the MELPA guide is intended to be more general and less about fine details like this)

I agree, in fact this is why I asked for a link. MELPA does refer to third-party style guides like the one I quoted, but I feared that if there were some MELPA-specific guide I missed altogether, then my code would have many more issues.

Thanks for answering my questions. I will make the proposed changes to the conditionals in the next days.

[alphapapa]

I agree, in fact this is why I asked for a link.

Well, if you're not comfortable taking my word for it, you can search the MELPA repo's pull requests for comments by Steve, where he frequently suggests it. Or you can ignore it altogether, because it is your package, so you can code according to your preferences. :)

[sinic]

The suggested changes are in, along with some simplifications made possible by functions in subr-x.

Well, if you're not comfortable taking my word for it, you can search the MELPA repo's pull requests for comments by Steve, where he frequently suggests it.

I no longer need convincing, but I found one of the comments you mentioned and can now refer to it if I open a pull request at the style guide. Thanks again (and sorry for accidentally editing one of your earlier comments, instead of mine!).

[alphapapa]

No problem, and you're welcome. FYI I made one further suggestion here: https://github.com/sinic/ednc/commit/80b0622f63d9cbe9eaaea9782bbb83c82439e4a1#r43971417