Re: [PATCH] piem-am: Order attached patches by file name prefix

[Kyle Meyer <kyle@kyleam.com>]

Ihor Radchenko writes:

> I only have some minor comments on the code.

Thanks for the feedback.

>>  (defun piem-gnus-am-ready-mbox ()
>>    "Return a function that inserts an am-ready mbox.
>> +
>>  If the buffer has any MIME parts that look like a patch, use
>> -those parts' contents (in order) as the mbox.  Otherwise, use the
>> -message itself if it looks like a patch."
>> +those parts' contents as the mbox, ordering the patches based on
>> +the number at the start of the file name.  If none of the file
>> +names start with a number, retain the original order of the
>> +attachments.
>> ...
>>  (defun piem-notmuch-am-ready-mbox ()
>>    "Return a function that inserts an am-ready mbox.
>> +
>>  If the buffer has any MIME parts that look like a patch, use
>> -those parts' contents (in order) as the mbox.  Otherwise, use the
>> -message itself if it looks like a patch."
>> +those parts' contents as the mbox, ordering the patches based on
>> +the number at the start of the file name.  If none of the file
>> +names start with a number, retain the original order of the
>> +attachments.
>
> There is certainly some code duplication going on here (;

For the two repeated blocks of the docstrings, I'm okay with a bit of
repeated documentation in this situation.  Another option would be to
put something about the attachment order in the docstring of
piem-am-ready-mbox-functions, but that moves it farther from the
relevant context.  (Or to just not document it at all.)

If you're commenting more generally on the _code_ of
piem-gnus-am-ready-mbox and piem-notmuch-am-ready-mbox, I'm open to
suggestions for extracting out any remaining shared bits, but my hunch
is that most directions would obscure things.

>> +  (when (listp handle)
>> +    (let ((type (mm-handle-media-type handle))
>> +          (filename (mm-handle-filename handle)))
>> +      (and (or (member type '("text/x-diff" "text/x-patch"))
>> +               (and filename
>> +                    (equal type "text/plain")
>> +                    (string-suffix-p ".patch" filename t)))
>> +           (with-temp-buffer
>> +             (mm-display-inline handle)
>> +             (cons
>> +              (string-to-number filename)
>> +              (buffer-substring-no-properties (point-min) (point-max))))))))
>
> Why using (and ...)? It feels like (when ...) is more appropriate in
> this context.

I like the (very soft and fuzzy) style guideline of using `and` to
signal the return value is the primary thing of interest and `when` to
signal the code is executed for side effects.  I'm not sure who I first
saw describe this convention [*], when I started to share the
preference, or how consistent I am in how I apply it.

Due to indentation, nesting, and/or grouping, I think there are cases
where it's more readable to use `when`, such as the top-level `when`
above.  Perhaps on a different night I would have felt the same about
the `and` you mentioned and flipped it to a `when`.

Is there a particular reason you prefer `when` in the above case?


[*] I'm not sure how widely this preference is shared among Elisp
    coders.  Perhaps it mostly comes from elsewhere, like Common Lisp
    (<https://www.cs.cmu.edu/Groups/AI/html/cltl/clm/node84.html>):

      As a matter of style, `when` is normally used to conditionally
      produce some side effects, and the value of the `when` form is
      normally not used. If the value is relevant, then it may be
      stylistically more appropriate to use `and` or `if`.

    I recall seeing Nicolas state it several times in reviews on the Org
    list.  Some examples:

    - https://list.orgmode.org/?q=f%3Anicolas+AND+nq%3Awhen+ADJ+nq%3Aside
    - https://list.orgmode.org/87d23sdtod.fsf@nicolasgoaziou.fr/