From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp10.migadu.com ([2001:41d0:2:267::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms4r.migadu.com with LMTPS id 8MGxHlSzyWL0sAAATbCpxg (envelope-from ) for ; Sat, 09 Jul 2022 18:56:52 +0200 Received: from out0.migadu.com ([2001:41d0:2:267::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp10.migadu.com with LMTPS id eKW/HVSzyWIThgAAG6o9tA (envelope-from ) for ; Sat, 09 Jul 2022 18:56:52 +0200 X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kyleam.com; s=key1; t=1657385812; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to; bh=0v/3+D8DZRmqIxMRu6nLxsQnNJCDi+Hxt5VRPzYGb/0=; b=MXN7jjqDezev1c/UecuqPFFU7hqIWGP24Dhl92PAvSPbvrB0byqX00SgtJLAK4xoCB/kRu HhAq/OiVmMp9yA1jQxWImM+ZEMWvURgkHSzIplGkh9Ntss8CRlwVy2mhVyECf0nVCPIH2d U6KTU8oyvFMEyJOQoKYomsjBvjPAVzgWXoL3wPkd0imlIsqm6sBfQYae0IPZ9GykqrXksN SogCuJYhScNRApaAAyCezCQUYBIY8uxYtRBe29Y2RqyVxtAvDM1nmqNh1J5dZQmZrtWG1X bfY3cvIea64oSGFoVhykARJBDZund2beYOI3KR2rS8zi0B2dajx1oIsXUSF/hQ== From: Kyle Meyer To: Ihor Radchenko Cc: piem@inbox.kyleam.com Subject: Re: [PATCH] piem-am: Order attached patches by file name prefix In-Reply-To: <87fsjaspkl.fsf@localhost> Date: Sat, 09 Jul 2022 12:56:49 -0400 Message-ID: <87czee6x7y.fsf@kyleam.com> MIME-Version: 1.0 Content-Type: text/plain X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: kyleam.com X-TUID: MfrEC1wTt4jj 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 (): 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/