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

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

On Sat, 6 May 2017, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  mvoteiza@udel.edu,  emacs-devel@gnu.org
>> Date: Sat, 06 May 2017 22:51:26 +0900
>>
>>> Add optional regexp for subr-x.el trimming functions
>> I would say something like:
>> Allow user regexp in string trimming functions.
>
> FWIW, I don't see much difference between these two.  And the option
> is not really for "users", it's for Lisp programs, right?
The latter try to summary the issue, that is 'what'.  The former sounds 
more like the implementation to fix the issue, i.e., 'how'.

>>> +(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.
>
> A matter of personal style, IMO.
Emacs might have a prefered style for this, and promote it.

>> 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)
>
> I wouldn't make comments like this so late in the review process.
It's just a rename of local variables.  It's really a minor issue.

>> 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)
>
> Again, personal style issue, IMO.
Just wondering what is your personal style here.
Emacs might have a prefered style for this as well.  It's nicer to read
uniform style sources.