Re: [RFC PATCH] Add :maildir keyword to piem-inboxes

[Kyle Meyer <kyle@kyleam.com>]

Xinglu Chen writes:

> Previously the user was able to configure a maildir to inject threads
> into, but the user might want the maildir to be different depending
> on which mailing list the threads was coming from.

Yeah, I suspect that'd be a pretty common preference.  (From the current
limitation, you probably guessed that I'm a lumper when it comes to
mailing list messages.)

> With the `:maildir` keyword, users can configure the maildir on a
> per-list basis.  If there is not `:maildir` configured for a mailing
> list, it will fallback to the value of `piem-maildir-directory`.

Sounds good.

> ---
> I made `piem--write-mbox-to-maildir` take the maildir as an argument,
> otherwise `piem-inbox-maildir-directory` would return nil because it is
> not in a Gnus och Notmuch buffer when being called in
> `piem--write-mbox-to-maildir`.

Make sense.

>  Documentation/piem.texi |  3 +-
>  piem.el                 | 90 ++++++++++++++++++++++++-----------------
>  2 files changed, 55 insertions(+), 38 deletions(-)
>
> diff --git a/Documentation/piem.texi b/Documentation/piem.texi
> index ebd756d..9f8779f 100644
> --- a/Documentation/piem.texi
> +++ b/Documentation/piem.texi
> @@ -410,7 +410,8 @@
>  regular mail but are following a project via NNTP in Gnus).  In this
>  case, you can use the command @code{piem-inject-thread-into-maildir} to
>  move the thread's messages into a local Maildir directory
> -(@code{piem-maildir-directory}).  By default the command downloads the
> +(@code{piem-maildir-directory}, or the @code{:maildir} for the
> +current inbox).  By default the command downloads the

Hmm, with :maildir coming second, I think I'd interpret the priority to
be opposite of what it is.  Perhaps something closer to the phrasing you
used in the commit message:

  ... into a local Maildir directory specified by the current inbox's
  @code{:maildir} value in @code{piem-inboxes}, falling back to
  @code{piem-maildir-directory}.

>  entire thread for the message ID associated with the current buffer.  A
>  prefix argument restricts the download to only the message.
>  
> diff --git a/piem.el b/piem.el
> index 56f0b54..fd0a41b 100644
> --- a/piem.el
> +++ b/piem.el
[...]
> @@ -588,7 +603,7 @@ (defun piem--write-mbox-to-maildir ()
>  
>  ;;;###autoload
>  (defun piem-inject-thread-into-maildir (mid &optional message-only)
> -  "Inject thread containing MID into `piem-maildir-directory'.
> +  "Inject thread containing MID into `piem-inbox-maildir-directory'.
>  
>  If prefix argument MESSAGE-ONLY is non-nil, inject just the
>  message for MID, not the entire thread.
> @@ -599,36 +614,37 @@ (defun piem-inject-thread-into-maildir (mid &optional message-only)
>     (list (or (piem-mid)
>               (user-error "No message ID found for the current buffer"))
>           current-prefix-arg))
> -  (cond
> -   ((not piem-maildir-directory)
> -    (user-error "`piem-maildir-directory' is not configured"))
> -   ((not (piem-maildir-dir-is-maildir-p piem-maildir-directory))
> -    (user-error
> -     "`piem-maildir-directory' does not look like a Maildir directory"))
> -   ((not (or message-only (piem-check-gunzip)))
> -    (user-error "gunzip executable not found")))
> -  (when-let ((url (concat (piem-mid-url mid)
> -                          (if message-only "/raw" "/t.mbox.gz")))
> -             (buffer (url-retrieve-synchronously url 'silent)))
[...]
> -      (kill-buffer buffer))))
> +  (let ((maildir-directory (piem-inbox-maildir-directory)))
> +    (cond
> +     ((not maildir-directory)
> +      (user-error "`maildir-directory' is not configured"))

Unlike the previous piem-maildir-directory, maildir-directory is local
to this function, so a user that sees this message can't learn more with
`C-h v' and friends.  How about

  "No directory returned by `piem-inbox-maildir-directory'"

instead?

> +     ((not (piem-maildir-dir-is-maildir-p maildir-directory))
> +      (user-error
> +       "`maildir-directory' does not look like a Maildir directory"))

Similar comment here.  Perhaps

  (user-error
   "Does not look like a Maildir directory: %s" maildir-directory)


Anyway, those are all minor comments.  Looking very nice.  Thanks.