Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Tino Calancha <tino.calancha@gmail.com>]

Jean-Christophe Helary <jean.christophe.helary@gmail.com> writes:

> Ok, I think I have something clean.
> (btw, I'm done with the copyleft paperwork)
>
> Here is the diff, and a potential log message.
>
> Jean-Christophe 

Thank you very much!
Some comments below.

===========================
>Add optional regexp for subr-x.el trimming functions
I would say something like:
Allow user regexp in string trimming functions.

>* lisp/emacs-lisp/subr-x.el (string-trim-left) (string-trim-right)
                                              ^^^
This must be:
* lisp/emacs-lisp/subr-x.el (string-trim-left, string-trim-right)

>(string-trim): add optional regexp that defaults on the original behavior.
                ^^^
Sentence must start in capital letter.

I would add a separated line for `string-trim', because there we add
two regexp's not just one.

So i would say something like:
===============================================================================
Allow user regexp in string trimming functions

* lisp/emacs-lisp/subr-x.el (string-trim-left) (string-trim-right):
Add optional arg REGEXP.
(string-trim): Add optional args REGEXP-BEG, REGEXP-END.
See discussion in:
https://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00047.html
===============================================================================

>-(defsubst string-trim-left (string)
>-  "Remove leading whitespace from STRING."
>-  (if (string-match "\\`[ \t\n\r]+" string)
>+(defsubst string-trim-left (string &optional regexp)
>+  "Trim STRING of leading string matching REGEXP.
>+
>+REGEXP defaults to \"[ \\t\\n\\r]+\"."
We don't need a empty line inside such short docstrings.

>-(defsubst string-trim-right (string)
>-  "Remove trailing whitespace from STRING."
>-  (if (string-match "[ \t\n\r]+\\'" string)
>+(defsubst string-trim-right (string &optional regexp)
>+  "Trim STRING of trailing string matching REGEXP.
>+
>+REGEXP defaults to  \"[ \\t\\n\\r]+\"."
>+  
>+  (if (string-match (concat (or regexp "[ \t\n\r]+") "\\'") string)
We must drop the empty line between the docstrings and the body of
`string-trim-left' and `string-trim-right'.
 
>-(defsubst string-trim (string)
>-  "Remove leading and trailing whitespace from STRING."
>-  (string-trim-left (string-trim-right string)))
>+(defsubst string-trim (string &optional trim-left trim-right)
>+  "Trim STRING of leading and trailing strings matching TRIM-LEFT and TRIM-RIGHT.
>+
>+TRIM-LEFT and TRIM-RIGHT default to \"[ \\t\\n\\r]+\"."
>+
>+  (string-trim-left (string-trim-right string trim-right) trim-left))
I feel like too much 'trim', 'left' and 'right' around.  It's distracting.
I suggest something like:
(defsubst string-trim (string &optional regexp-beg regexp-end)
or
(defsubst string-trim (string &optional regexp-l regexp-r)

[Minor comment]
I find it more legible written as:
(string-trim-left
 (string-trim-right string regexp-end)
 regexp-beg)

than as:
(string-trim-left (string-trim-right string regexp-end) regexp-beg)

Cheers,
Tino