discussion and development of piem
 help / color / mirror / code / Atom feed
From: Kyle Meyer <kyle@kyleam.com>
To: Xinglu Chen <public@yoctocell.xyz>
Cc: piem@inbox.kyleam.com
Subject: Re: [RFC PATCH] Add :maildir keyword to piem-inboxes
Date: Thu, 11 Mar 2021 18:43:14 -0500	[thread overview]
Message-ID: <87im5x9rzh.fsf@kyleam.com> (raw)
In-Reply-To: <9ff4c94928b0cd0d223799fa8a978d94e545522e.1615460587.git.public@yoctocell.xyz>

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.

  reply	other threads:[~2021-03-11 23:43 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-11 11:06 Xinglu Chen
2021-03-11 23:43 ` Kyle Meyer [this message]
2021-03-12 16:43   ` Xinglu Chen
2021-03-12 16:55 ` [PATCH v2] piem: " Xinglu Chen
2021-03-13  1:18   ` Kyle Meyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://git.kyleam.com/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87im5x9rzh.fsf@kyleam.com \
    --to=kyle@kyleam.com \
    --cc=piem@inbox.kyleam.com \
    --cc=public@yoctocell.xyz \
    --subject='Re: [RFC PATCH] Add :maildir keyword to piem-inboxes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://git.kyleam.com/piem/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).