[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Tino Calancha
Subject: Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification
Date: Sat, 06 May 2017 22:51:26 +0900
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux)

Jean-Christophe Helary <address@hidden> 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:

>-(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-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)
(defsubst string-trim (string &optional regexp-l regexp-r)

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

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]