Re: [PATCH] New with-file-buffer macro

[Arthur Miller]

Adam Porter <adam@alphapapa.net> writes:

> Hi,
>
> Attached is a patch which adds a new macro, with-file-buffer.  It's
> similar to the macro with-temp-file, and it simplifies some common
> cases, like reading a file's contents into a buffer and returning a
> value from forms evaluated in its buffer, writing to a file that
> shouldn't already exist, overwriting an existing file, etc.
>
> For example, here's a recently updated function in package-build.el
> which uses with-temp-buffer and write-region:
>
> #+BEGIN_SRC elisp
>   (defun package-build--write-pkg-readme (name files directory)
>     (when-let ((commentary
>                 (let* ((file (concat name ".el"))
>                        (file (or (car (rassoc file files)) file))
>                        (file (and file (expand-file-name file directory))))
>                   (and (file-exists-p file)
>                        (lm-commentary file)))))
>       (with-temp-buffer
>         (if (>= emacs-major-version 27)
>             (insert commentary)
>           ;; Taken from 27.1's `lm-commentary'.
>           (insert
>            (replace-regexp-in-string ; Get rid of...
>             "[[:blank:]]*$" "" ; trailing white-space
>             (replace-regexp-in-string
>              (format "%s\\|%s\\|%s"
>                      ;; commentary header
>                      (concat "^;;;[[:blank:]]*\\("
>                              lm-commentary-header
>                              "\\):[[:blank:]\n]*")
>                      "^;;[[:blank:]]*" ; double semicolon prefix
>                      "[[:blank:]\n]*\\'") ; trailing new-lines
>              "" commentary))))
>         (unless (= (char-before) ?\n)
>           (insert ?\n))
>         (let ((coding-system-for-write buffer-file-coding-system))
>           (write-region nil nil
>                         (expand-file-name (concat name "-readme.txt")
>                                           package-build-archive-dir))))))
> #+END_SRC
>
>
> Here's how it could work using this new with-file-buffer macro:
>
> #+BEGIN_SRC elisp
>   (defun package-build--write-pkg-readme (name files directory)
>     (when-let ((commentary
>                 (let* ((file (concat name ".el"))
>                        (file (or (car (rassoc file files)) file))
>                        (file (and file (expand-file-name file directory))))
>                   (and (file-exists-p file)
>                        (lm-commentary file)))))
>       (let ((coding-system-for-write buffer-file-coding-system))
>         (with-file-buffer (expand-file-name (concat name "-readme.txt")
>                                             package-build-archive-dir)
>             (:insert nil :write t :overwrite t)
>           (if (>= emacs-major-version 27)
>               (insert commentary)
>             ;; Taken from 27.1's `lm-commentary'.
>             (insert
>              (replace-regexp-in-string ; Get rid of...
>               "[[:blank:]]*$" "" ; trailing white-space
>               (replace-regexp-in-string
>                (format "%s\\|%s\\|%s"
>                        ;; commentary header
>                        (concat "^;;;[[:blank:]]*\\("
>                                lm-commentary-header
>                                "\\):[[:blank:]\n]*")
>                        "^;;[[:blank:]]*" ; double semicolon prefix
>                        "[[:blank:]\n]*\\'") ; trailing new-lines
>                "" commentary))))
>           (unless (= (char-before) ?\n)
>             (insert ?\n))))))
> #+END_SRC
>
> This example isn't the most dramatically different, but it shows a few
> improvements: the options list after the filename shows clearly what the
> form does (write a file, overwriting if it already exists), whereas
> calling write-region directly requires reading to the end of the body
> forms (which might not fit within the window); and the options list also
> makes arguments to write-region easier to use, as well as making it easy
> to control write-region-inhibit-fsync and file-precious-flag.  Without
> this macro, these operations and options are, IMO, harder to use and
> easy to overlook, so I think this would be a worthwhile addition.
>
> If this idea seems useful, I'm sure various improvements may be made, so
> I would appreciate any feedback.  For example, it might be better for
> ":insert nil" to be the default, so ":insert t" would be specified to
> read the file's contents into the buffer.
>
> Thanks,
> Adam
>
> From 37df936cc1c9e3f630a060c8347a4aeba46fc7d0 Mon Sep 17 00:00:00 2001
> From: Adam Porter <adam@alphapapa.net>
> Date: Thu, 31 Dec 2020 12:56:25 -0600
> Subject: [PATCH] * lisp/subr.el (with-file-buffer): New macro.
>
> * lisp/subr.el (with-file-buffer): New macro.
> * doc/lispref/files.texi (Writing to Files): Document it.
> * etc/NEWS: Add news entry.
> ---
>  doc/lispref/files.texi | 48 ++++++++++++++++++++++++++++++++++++++++++
>  etc/NEWS               |  6 ++++++
>  lisp/subr.el           | 57 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>
> diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
> index 6949ca2..1617bbf 100644
> --- a/doc/lispref/files.texi
> +++ b/doc/lispref/files.texi
> @@ -708,6 +708,54 @@ Writing to Files
>  (@pxref{Buffer List}).
>  @end defmac
>  
> +@defmac with-file-buffer file options body@dots{}
> +@anchor{Definition of with-file-buffer}
> +Like the @code{with-temp-file} macro (which see), the
> +@code{with-file-buffer} macro evaluates the @var{body} forms with a
> +temporary buffer as the current buffer and then returns the value of
> +the last form in @var{body}.  The @var{options} offer control over
> +various aspects of the operations:
> +
> +@table @code
> +@item :insert
> +When non-nil (the default, when unspecified), insert @var{file}'s contents
> +before evaluating @var{body}, leaving point before the contents.
> +
> +@item :must-exist
> +When non-nil, signal an error if @var{file} does not exist.
> +
> +@item :write
> +When non-nil, write the contents of the buffer to @var{file} after
> +evaluating @var{body}.
> +
> +@item :overwrite
> +When nil (the default, when unspecified), signal an error instead of
> +overwriting an existing file.  If @code{ask}, ask for confirmation
> +before overwriting an existing file.  If @code{t}, overwrite
> +@var{file} unconditionally.
> +
> +@item :visit
> +Passed to function @code{write-region}, which see.
> +
> +@item :lockname
> +Passed to function @code{write-region}, which see.
> +
> +@item :append
> +Passed to function @code{write-region}, which see.  (When using this
> +option, you will probably want to specify @code{:insert nil} as well,
> +otherwise the file's contents would be duplicated.)
> +
> +@item :fsync
> +When non-nil (the default, when unspecified), bind
> +@var{write-region-inhibit-fsync} (which see) to this value.
> +
> +@item :precious
> +Bind @var{file-precious-flag} (which see) to this
> +value (when unspecified, nil).
> +@end table
> +
> +@end defmac
> +
>  @node File Locks
>  @section File Locks
>  @cindex file locks
> diff --git a/etc/NEWS b/etc/NEWS
> index 62907a6..9b8288f 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2311,6 +2311,12 @@ locales.  They are also available as aliases 
> 'ebcdic-cp-*' (e.g.,
>  'cp278' for 'ibm278').  There are also new charsets 'ibm2xx' to
>  support these coding-systems.
>  
> +---
> +** New macro 'with-file-buffer'.
> +The new macro 'with-file-buffer' simplifies operating on the contents
> +of files and/or writing to them, compared to using 'with-temp-buffer',
> +'insert-file-contents', 'write-region', and various options
> +separately.
>  
>  * Changes in Emacs 28.1 on Non-Free Operating Systems
>  
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 77b142c..0f6da02 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -3992,6 +3992,63 @@ with-temp-buffer
>             (and (buffer-name ,temp-buffer)
>                  (kill-buffer ,temp-buffer)))))))
>  
> +(defmacro with-file-buffer (file options &rest body)
> +  "Evaluate BODY and return its value in a temp buffer for FILE.
> +OPTIONS is a plist of the following options:
> +
> +`:insert': When non-nil (the default, when unspecified), insert
> +file's contents before evaluating BODY, leaving point before the
> +contents.
> +
> +`:must-exist': When non-nil, signal an error if no file exists at
> +FILE.
> +
> +`:write': When non-nil, write the contents of the buffer to FILE
> +after evaluating BODY.
> +
> +`:overwrite': When nil (the default, when unspecified), signal an
> +error instead of overwriting an existing file at FILE.  If `ask',
> +ask for confirmation before overwriting an existing file.  If t,
> +overwrite a file at FILE unconditionally.
> +
> +`:visit': Passed to function `write-region', which see.
> +
> +`:lockname:' Passed to function `write-region', which see.
> +
> +`:append': Passed to function `write-region', which see.  (When
> +using this option, you will probably want to specify `:insert
> +nil' as well.)
> +
> +`:fsync': When non-nil (the default, when unspecified), bind
> +`write-region-inhibit-fsync' (which see) to this value.
> +
> +`:precious': Bind `file-precious-flag' (which see) to this
> +value (when unspecified, nil)."
> +  (declare (indent 2) (debug (stringp form body)))
> +  `(let ((write-region-inhibit-fsync ,(when (plist-member options :fsync)
> +                                        (not (plist-get options :fsync))))
> +         (file-precious-flag ,(plist-get options :precious)))
> +     (with-temp-buffer
> +       ,(when (or (not (plist-member options :insert))
> +                  (plist-get options :insert))
> +          `(if (file-readable-p ,file)
> +               (save-excursion
> +                 (insert-file-contents ,file))
> +             (when ,(plist-get options :must-exist)
> +               (error "File not readable: %s" ,file))))
> +       (prog1
> +           (progn
> +             ,@body)
> +         ,(when (plist-get options :write)
> +            `(write-region nil nil ,file
> +                           ,(plist-get options :append)
> +                           ,(plist-get options :visit)
> +                           ,(plist-get options :lockname)
> +                           ,(pcase-exhaustive (plist-get options :overwrite)
> +                              ('nil ''excl)
> +                              ((or 'ask ''ask) ''ask)
> +                              ('t nil))))))))
> +
>  (defmacro with-silent-modifications (&rest body)
>    "Execute BODY, pretending it does not modify the buffer.
>  This macro is typically used around modifications of
I am not an expert in Lisp; but I personally think those 'with-*' macros
are more idiomatic ot Lisp. They are also communicate clearly the scope
where a variable (in this case buffer or file are used). I understand
Eli's remark that thay don't remove much to the burden of user in LoCs
the user have to write, but they do document the user intention and
helps writing better self-documenting code. I compare this personally to
std::copy from C++ vs raw for-loop. std::copy does not make us write
less code; usually if the loop body is big we will refactor it in a
callback, but it does document the intention. Elisp is not as efficient
as c++ compiler to eliminate the overhead, but macros will be expanded
when byte-compiled, so it really is just the memory overhead of defining
the body of mcaro.

I personally use, beside the macro I posted in documentation thread,
those two macros, which are admittedly bad, but works for me personally:

#+BEGIN_SRC elisp
(defmacro with-file-prepend (file &rest body)
  (declare (indent 1) (debug t))
  `(let ((buffer (get-buffer-create ,file)))
     (unwind-protect
         (prog1 
             (with-current-buffer buffer
               ,@body
               (goto-char (point-max))
               (if (file-readable-p ,file)
                 (insert-file-contents ,file))
           (with-current-buffer buffer
             (write-region nil nil ,file nil 0)))
       (and (buffer-name buffer)
            (kill-buffer buffer))))))

(defmacro with-file-append (file &rest body)
  (declare (indent 1) (debug t))
  `(let ((buffer (get-buffer-create ,file)))
     (unwind-protect
         (prog1 
             (with-current-buffer buffer
               (if (file-readable-p ,file)
                 (insert-file-contents ,file))
               (goto-char (point-max))
               ,@body)
           (with-current-buffer buffer
             (write-region nil nil ,file nil 0)))
       (and (buffer-name buffer)
            (kill-buffer buffer)))))
#+END_SRC

I can then use them like:

#+BEGIN_SRC elisp
(with-file-prepend "early-init.el"
        (insert (concat ";;; early-init.el -*- lexical-binding: t; -*-\n"
                        ";;; This file is machine generated by init file 
generator, don't edit\n"
                        ";;; manually, edit instead file init.org and generate 
new init file from it\n\n")))
    
      (with-file-append "init.el"
        (insert (concat "\n;; Local Variables:\n"
                        ";; byte-compile-warnings: (not free-vars 
unresolved))\n"                 
                        ";; End:\n")))
#+END_SRC

I don't suggest to use/includ those, just that I find it personally
handy, and more idiomatic then using say f.el or write-region directly
which is kind-of more C/Java-like APIs.

I personally am not qualified to say if plist implementation of
'with-file-buffer' macro is good or not; but I think the intention is
certainly good. If you are already buffing string library, make you
could buff file API too.