[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:
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
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, (continued)
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Mark Oteiza, 2017/05/02
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Jean-Christophe Helary, 2017/05/02
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Jean-Christophe Helary, 2017/05/05
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Tino Calancha, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Jean-Christophe Helary, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Eli Zaretskii, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Jean-Christophe Helary, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Eli Zaretskii, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Jean-Christophe Helary, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Jean-Christophe Helary, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification,
Tino Calancha <=
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Eli Zaretskii, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Tino Calancha, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Eli Zaretskii, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Tino Calancha, 2017/05/07
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Johan Bockgård, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Eli Zaretskii, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Johan Bockgård, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Eli Zaretskii, 2017/05/06
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Johan Bockgård, 2017/05/07
- Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification, Eli Zaretskii, 2017/05/07