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

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

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

Following the discussion about how trimming in subr-x.el did not allow for non-default regex, I have this small modification to the 3 trimming functions found therein.

Basically:
1) I extracted the default regex and assigned it to string-trim-default-regex
2) I declared optional "trim-left" and "trim-right" arguments respectively for the string-trim-left and string-trim-right functions, and declared both optional for the general string-trim function

The documentation strings are pretty much taken from subr.el so maybe the wording is not the best.

Please advise.

Jean-Christophe


[-- Attachment #2: subr-x.el.diff --]
[-- Type: application/octet-stream, Size: 2083 bytes --]

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 440213eb38..188f314871 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -179,21 +179,36 @@ VARLIST can just be a plain tuple.
 
 (define-obsolete-function-alias 'string-reverse 'reverse "25.1")
 
-(defsubst string-trim-left (string)
-  "Remove leading whitespace from STRING."
-  (if (string-match "\\`[ \t\n\r]+" string)
+(defconst string-trim-default-regex "[ \t\n\r]+")
+  "The default value of separators for `string-trim'."
+
+(defsubst string-trim-left (string &optional trim-left)
+  "Trim STRING of leading whitespace matching TRIM-LEFT.
+
+If TRIM-LEFT is non-nil, it should be a regular expression. If nil,
+it defaults to `string-trim-default-regex', normally \"[ \t\n\r]+\"."
+  
+  (if (string-match (concat "\\`" (or  trim-left string-trim-default-regex)) string)
       (replace-match "" t t string)
     string))
 
-(defsubst string-trim-right (string)
-  "Remove trailing whitespace from STRING."
-  (if (string-match "[ \t\n\r]+\\'" string)
+(defsubst string-trim-right (string &optional trim-right)
+  "Trim STRING of trailing whitespace matching TRIM-RIGHT.
+
+If TRIM-RIGHT is non-nil, it should be a regular expression. If nil,
+it defaults to `string-trim-default-regex', normally \"[ \t\n\r]+\"."
+  
+  (if (string-match (concat (or trim-right string-trim-default-regex) "\\'") string)
       (replace-match "" t t string)
     string))
 
-(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 whitespace matching TRIM-LEFT and TRIM-RIGHT.
+
+If TRIM-LEFT and TRIM-RIGHT are non-nil, they should be a regular expression. If nil,
+they default to `string-trim-default-regex', normally \"[ \t\n\r]+\"."
+
+  (string-trim-left (string-trim-right string trim-right) trim-left))
 
 (defsubst string-blank-p (string)
   "Check whether STRING is either empty or only whitespace."

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

[Clément Pit-Claudel]

Looks good.  The stirng could be called string-trim-regexp (no need for default), though, I think.

On 2017-05-02 05:34, Jean-Christophe Helary wrote:
> Following the discussion about how trimming in subr-x.el did not allow for non-default regex, I have this small modification to the 3 trimming functions found therein.
> 
> Basically:
> 1) I extracted the default regex and assigned it to string-trim-default-regex
> 2) I declared optional "trim-left" and "trim-right" arguments respectively for the string-trim-left and string-trim-right functions, and declared both optional for the general string-trim function
> 
> The documentation strings are pretty much taken from subr.el so maybe the wording is not the best.

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

[Jean-Christophe Helary]

> On May 3, 2017, at 0:41, Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:
> 
> Looks good.  The stirng could be called string-trim-regexp (no need for default), though, I think.

I copied the name pattern from subr.el (split-string). I have no strong opinion about names but I thought it was good to keep some consistency between similar functions (although I understand that not all default values are named "default"...)

Jean-Christophe

> 
> On 2017-05-02 05:34, Jean-Christophe Helary wrote:
>> Following the discussion about how trimming in subr-x.el did not allow for non-default regex, I have this small modification to the 3 trimming functions found therein.
>> 
>> Basically:
>> 1) I extracted the default regex and assigned it to string-trim-default-regex

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

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Tue, 2 May 2017 18:34:17 +0900
> 
> Following the discussion about how trimming in subr-x.el did not allow for non-default regex, I have this small modification to the 3 trimming functions found therein.
> 
> Basically:
> 1) I extracted the default regex and assigned it to string-trim-default-regex
> 2) I declared optional "trim-left" and "trim-right" arguments respectively for the string-trim-left and string-trim-right functions, and declared both optional for the general string-trim function

Thanks.

Do we really have use cases for string-trim where trim-left and
trim-right should be different?  If not, let's give it only one
optional argument, not 2.

> +(defsubst string-trim-left (string &optional trim-left)
> +  "Trim STRING of leading whitespace matching TRIM-LEFT.

Since TRIM-LEFT could be anything, the "whitespace" part is no longer
accurate.  Suggest to use "delimiters" instead.

> +If TRIM-LEFT is non-nil, it should be a regular expression. If nil,
> +it defaults to `string-trim-default-regex', normally \"[ \t\n\r]+\"."

This is better worded like this:

  TRIM-LEFT should be a regular expression, and defaults to
  `string-trim-default-regex' if omitted or nil.

> +(defsubst string-trim-right (string &optional trim-right)
> +  "Trim STRING of trailing whitespace matching TRIM-RIGHT.
> +
> +If TRIM-RIGHT is non-nil, it should be a regular expression. If nil,
> +it defaults to `string-trim-default-regex', normally \"[ \t\n\r]+\"."

Likewise.

> +(defsubst string-trim (string &optional trim-left trim-right)
> +  "Trim STRING of leading and trailing whitespace matching TRIM-LEFT and TRIM-RIGHT.

Please make the first line shorter than 80 characters.

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

[Jean-Christophe Helary]

> On May 3, 2017, at 2:16, Eli Zaretskii <eliz@gnu.org> wrote:
>> 2) I declared optional "trim-left" and "trim-right" arguments respectively for the string-trim-left and string-trim-right functions, and declared both optional for the general string-trim function
> 
> Do we really have use cases for string-trim where trim-left and
> trim-right should be different?  If not, let's give it only one
> optional argument, not 2.

I have no clear use case because of my lack of programming experience, but I can see how TRIM-LEFT could be used to remove the beginning of a path and TRIM-RIGHT could be used to remove a file extension, for ex.

>> +(defsubst string-trim-left (string &optional trim-left)
>> +  "Trim STRING of leading whitespace matching TRIM-LEFT.
> 
> Since TRIM-LEFT could be anything, the "whitespace" part is no longer
> accurate.  Suggest to use "delimiters" instead.

I overlooked that. In fact, since, as you correctly point out, a regex can be anything I think "characters" or "the string" is even better. It seems to me that "delimiter" fits better the concept of "splitting" than the concept of "trimming".

>> +If TRIM-LEFT is non-nil, it should be a regular expression. If nil,
>> +it defaults to `string-trim-default-regex', normally \"[ \t\n\r]+\"."
> 
> This is better worded like this:
> 
>  TRIM-LEFT should be a regular expression, and defaults to
>  `string-trim-default-regex' if omitted or nil.

Thank you. I tried to stick to the wording found in subr.el (split-string)... :-) I'll do as you suggest.

>> +(defsubst string-trim (string &optional trim-left trim-right)
>> +  "Trim STRING of leading and trailing whitespace matching TRIM-LEFT and TRIM-RIGHT.
> 
> Please make the first line shorter than 80 characters.

I overlooked that one. Sorry.

Jean-Christophe 

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

[Mark Oteiza]

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

> Following the discussion about how trimming in subr-x.el did not allow
> for non-default regex, I have this small modification to the 3
> trimming functions found therein.
>
> Basically:
> 1) I extracted the default regex and assigned it to string-trim-default-regex
> 2) I declared optional "trim-left" and "trim-right" arguments
> respectively for the string-trim-left and string-trim-right functions,
> and declared both optional for the general string-trim function
>
> The documentation strings are pretty much taken from subr.el so maybe
> the wording is not the best.
>
> Please advise.

Basically everything in subr-x is either a macro or a defsubst, except
for read-multiple-choice (aside: why is this not in its own library?).

As such, most uses of subr-x are done with (eval-when-compile (require
'subr-x)).  Won't the use of a global variable break these?

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

[Jean-Christophe Helary]

> On May 3, 2017, at 8:11, Mark Oteiza <mvoteiza@udel.edu> wrote:

> Basically everything in subr-x is either a macro or a defsubst,
> 
> As such, most uses of subr-x are done with (eval-when-compile (require
> 'subr-x)).  Won't the use of a global variable break these?

(defconst string-trim-default-regex "[ \t\n\r]+")
  "The default value of the trimmed string for `string-trim'."

I have no idea what the effects would be. What would you suggest?

Jean-Christophe

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

[Jean-Christophe Helary]

Mark, do you have any suggestions regarding your remark?

Jean-Christophe 

> On May 3, 2017, at 10:13, Jean-Christophe Helary <jean.christophe.helary@gmail.com> wrote:
> 
> 
>> On May 3, 2017, at 8:11, Mark Oteiza <mvoteiza@udel.edu> wrote:
> 
>> Basically everything in subr-x is either a macro or a defsubst,
>> 
>> As such, most uses of subr-x are done with (eval-when-compile (require
>> 'subr-x)).  Won't the use of a global variable break these?
> 
> (defconst string-trim-default-regex "[ \t\n\r]+")
>  "The default value of the trimmed string for `string-trim'."
> 
> I have no idea what the effects would be. What would you suggest?
> 
> Jean-Christophe

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

[Tino Calancha]

[-- Attachment #1: Type: text/plain, Size: 3376 bytes --]



On Sat, 6 May 2017, Jean-Christophe Helary wrote:

>>> Basically everything in subr-x is either a macro or a defsubst,
>>>
>>> As such, most uses of subr-x are done with (eval-when-compile (require
>>> 'subr-x)).  Won't the use of a global variable break these?
>>
>> (defconst string-trim-default-regex "[ \t\n\r]+")
>>  "The default value of the trimmed string for `string-trim'."
>>
>> I have no idea what the effects would be. What would you suggest?

Hi Jean-Christophe,

Mark is right.  Your patch won't work in the following example:

Write a file '/tmp/test.el'
--8<-----------------------------cut here---------------start------------->8---
(eval-when-compile (require 'subr-x))
(defun test ()
   (message
    (string-trim "  Hi!  ")))
--8<-----------------------------cut here---------------end--------------->8---
M-! emacs -batch -eval '(byte-compile-file "/tmp/test.el")' && \
     emacs -batch -l /tmp/test.elc -eval "(test)" RET

;; Signals error: Symbol’s value as variable is void: string-trim-default-regex


Instead, something as follows would work:
--8<-----------------------------cut here---------------start------------->8---
commit dc4d7fa64c5cf4f3c3fa04182da974f2d0bc6499
Author: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
Date:   Sat May 6 13:24:03 2017 +0900

     Allow trim strings of whitespaces with a general regexp

     * lisp/emacs-lisp/subr-x.el (string-trim-left, string-trim-right):
     Add optional arg REGEXP.
     (string-trim): Add optional args TRIM-LEFT and TRIM-RIGHT.

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 440213eb38..5148e8b724 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -179,21 +179,29 @@ string-join

  (define-obsolete-function-alias 'string-reverse 'reverse "25.1")

-(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 whitespace matching REGEXP.
+
+REGEXP defaults to \"[ \t\n\r]+\"."
+  (if (string-match (concat "\\`" (or regexp "[ \t\n\r]+")) string)
        (replace-match "" t t string)
      string))

-(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 whitespace matching REGEXP.
+
+REGEXP defaults to \"[ \t\n\r]+\"."
+  (if (string-match (concat (or regexp "[ \t\n\r]+") "\\'") string)
        (replace-match "" t t string)
      string))

-(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 whitespace matching TRIM-LEFT and TRIM-RIGHT.
+
+Both, TRIM-RIGHT and TRIM-LEFT default to \"[ \t\n\r]+\"."
+  (string-trim-left
+   (string-trim-right string trim-right)
+   trim-left))

  (defsubst string-blank-p (string)
    "Check whether STRING is either empty or only whitespace."
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
  of 2017-05-05
Repository revision: 927dcbd2e6e0e53fcfb09296716e11c002ab1518


Cheers,
Tino

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

[Jean-Christophe Helary]

> On May 6, 2017, at 13:29, Tino Calancha <tino.calancha@gmail.com> wrote:
> 
> On Sat, 6 May 2017, Jean-Christophe Helary wrote:
> 
>>>> Basically everything in subr-x is either a macro or a defsubst,
>>>> 
>>>> As such, most uses of subr-x are done with (eval-when-compile (require
>>>> 'subr-x)).  Won't the use of a global variable break these?
>>> 
>>> (defconst string-trim-default-regex "[ \t\n\r]+")
>>> "The default value of the trimmed string for `string-trim'."
>>> 
>>> I have no idea what the effects would be. What would you suggest?

Thank you Tino for the suggestion.

> +(defsubst string-trim-left (string &optional regexp)
> +  "Trim STRING of leading whitespace matching REGEXP.
> +
> +REGEXP defaults to \"[ \t\n\r]+\"."
> +  (if (string-match (concat "\\`" (or regexp "[ \t\n\r]+")) string)
>       (replace-match "" t t string)
>     string))

What I did not like in the original design was that the default regexp was hard-coded (besides for the fact that it did not allow for options). I understand that your proposal *only* makes it the default, but I think it would be more elegant to have the default extracted from the code and visible to the user so as to allow for her to over-ride the default by resetting the value.

Isn't there a way to do that that would work with defsubst?

Jean-Christophe 

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

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Sat, 6 May 2017 18:02:37 +0900
> Cc: Mark Oteiza <mvoteiza@udel.edu>, emacs-devel <emacs-devel@gnu.org>
> 
> > +(defsubst string-trim-left (string &optional regexp)
> > +  "Trim STRING of leading whitespace matching REGEXP.
> > +
> > +REGEXP defaults to \"[ \t\n\r]+\"."
> > +  (if (string-match (concat "\\`" (or regexp "[ \t\n\r]+")) string)
> >       (replace-match "" t t string)
> >     string))
> 
> What I did not like in the original design was that the default regexp was hard-coded (besides for the fact that it did not allow for options). I understand that your proposal *only* makes it the default, but I think it would be more elegant to have the default extracted from the code and visible to the user so as to allow for her to over-ride the default by resetting the value.

Can you tell why you think that defconst is more elegant than a
hard-coded default?  I think they are basically the same.

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

[Jean-Christophe Helary]

> On May 6, 2017, at 18:22, Eli Zaretskii <eliz@gnu.org> wrote:

>> What I did not like in the original design was that the default regexp was hard-coded (besides for the fact that it did not allow for options). I understand that your proposal *only* makes it the default, but I think it would be more elegant to have the default extracted from the code and visible to the user so as to allow for her to over-ride the default by resetting the value.
> 
> Can you tell why you think that defconst is more elegant than a
> hard-coded default?  I think they are basically the same.

I was confused by the fact that a const is, well, a constant and is not to be overridden by the user.

My original idea was to put the regexp into a variable that *can* eventually be overridden by a user preference.

Jean-Christophe 

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

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Sat, 6 May 2017 19:33:10 +0900
> Cc: tino.calancha@gmail.com,
>  mvoteiza@udel.edu,
>  emacs-devel@gnu.org
> 
> My original idea was to put the regexp into a variable that *can* eventually be overridden by a user preference.

Does this really have legitimate use cases?  We are talking about a
very low-level function; overriding its default definition of
whitespace by a user option would affect every single use of this
function, and will most probably disrupt some code which uses it.

I think having a way for overriding the default programmatically
should be good enough, at least for now.  Let's not provide a
defcustom unless and until we have use cases for that which we want to
support.

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

[Jean-Christophe Helary]

Ok, I'm fine with that. Apologies for my lack of understanding of the issues.

Jean-Christophe

> On May 6, 2017, at 19:43, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> Date: Sat, 6 May 2017 19:33:10 +0900
>> Cc: tino.calancha@gmail.com,
>> mvoteiza@udel.edu,
>> emacs-devel@gnu.org
>> 
>> My original idea was to put the regexp into a variable that *can* eventually be overridden by a user preference.
> 
> Does this really have legitimate use cases?  We are talking about a
> very low-level function; overriding its default definition of
> whitespace by a user option would affect every single use of this
> function, and will most probably disrupt some code which uses it.
> 
> I think having a way for overriding the default programmatically
> should be good enough, at least for now.  Let's not provide a
> defcustom unless and until we have use cases for that which we want to
> support.

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

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

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 


===========================
Add optional regexp for subr-x.el trimming functions

* lisp/emacs-lisp/subr-x.el (string-trim-left) (string-trim-right) (string-trim): add optional regexp that defaults on the original behavior.
===========================



[-- Attachment #2: subr-x.el.diff --]
[-- Type: application/octet-stream, Size: 1614 bytes --]

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 440213eb38..40fe78c104 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -179,21 +179,30 @@ VARLIST can just be a plain tuple.
 
 (define-obsolete-function-alias 'string-reverse 'reverse "25.1")
 
-(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]+\"."
+  
+  (if (string-match (concat "\\`" (or  regexp "[ \t\n\r]+")) string)
       (replace-match "" t t string)
     string))
 
-(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)
       (replace-match "" t t string)
     string))
 
-(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))
 
 (defsubst string-blank-p (string)
   "Check whether STRING is either empty or only whitespace."

[-- Attachment #3: Type: text/plain, Size: 1120 bytes --]





> On May 6, 2017, at 20:02, Jean-Christophe Helary <jean.christophe.helary@gmail.com> wrote:
> 
> Ok, I'm fine with that. Apologies for my lack of understanding of the issues.
> 
> Jean-Christophe
> 
>> On May 6, 2017, at 19:43, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>>> Date: Sat, 6 May 2017 19:33:10 +0900
>>> Cc: tino.calancha@gmail.com,
>>> mvoteiza@udel.edu,
>>> emacs-devel@gnu.org
>>> 
>>> My original idea was to put the regexp into a variable that *can* eventually be overridden by a user preference.
>> 
>> Does this really have legitimate use cases?  We are talking about a
>> very low-level function; overriding its default definition of
>> whitespace by a user option would affect every single use of this
>> function, and will most probably disrupt some code which uses it.
>> 
>> I think having a way for overriding the default programmatically
>> should be good enough, at least for now.  Let's not provide a
>> defcustom unless and until we have use cases for that which we want to
>> support.
> 

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

[Tino Calancha]

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

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

[Eli Zaretskii]

> 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?

> >+(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.

> 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.

> 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.

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

[Tino Calancha]

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.

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

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sun, 7 May 2017 11:25:41 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, jean.christophe.helary@gmail.com, 
>     mvoteiza@udel.edu, Emacs developers <emacs-devel@gnu.org>
> 
> >> 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.

My personal style is to use the line as much as it allows me.

But I generally try not to impose my personal style on work done by
others.  I think every one of us is entitled to have the code they
contributed keep their style.

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

[Tino Calancha]

On Sun, 7 May 2017, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Sun, 7 May 2017 11:25:41 +0900 (JST)
>> cc: Tino Calancha <tino.calancha@gmail.com>, jean.christophe.helary@gmail.com,
>>     mvoteiza@udel.edu, Emacs developers <emacs-devel@gnu.org>
>>
>>>> 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.
>
> My personal style is to use the line as much as it allows me.
>
> But I generally try not to impose my personal style on work done by
> others.  I think every one of us is entitled to have the code they
> contributed keep their style.
Thanks.  No problem.

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

[Johan Bockgård]

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

> -(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]+\"."
> +  
> +  (if (string-match (concat "\\`" (or  regexp "[ \t\n\r]+")) string)

I think regexp should be surrounded by a grouping construct, like:

   (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")

And similarly for string-trim-right.

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

[Eli Zaretskii]

> From: Johan Bockgård <bojohan@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  mvoteiza@udel.edu,  emacs-devel@gnu.org,  tino.calancha@gmail.com
> Date: Sat, 06 May 2017 19:51:33 +0200
> 
> > +  (if (string-match (concat "\\`" (or  regexp "[ \t\n\r]+")) string)
> 
> I think regexp should be surrounded by a grouping construct, like:
> 
>    (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
> 
> And similarly for string-trim-right.

Rationale?

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

[Johan Bockgård]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Johan Bockgård <bojohan@gnu.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>, mvoteiza@udel.edu,
>> emacs-devel@gnu.org, tino.calancha@gmail.com
>> Date: Sat, 06 May 2017 19:51:33 +0200
>> 
>> > +  (if (string-match (concat "\\`" (or  regexp "[ \t\n\r]+")) string)
>> 
>> I think regexp should be surrounded by a grouping construct, like:
>> 
>>    (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
>> 
>> And similarly for string-trim-right.
>
> Rationale?

regexp can contain \|

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

[Eli Zaretskii]

> From: Johan Bockgård <bojohan@gnu.org>
> Cc: mvoteiza@udel.edu,  jean.christophe.helary@gmail.com,  tino.calancha@gmail.com,  emacs-devel@gnu.org
> Date: Sat, 06 May 2017 21:55:52 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Johan Bockgård <bojohan@gnu.org>
> >> Cc: Eli Zaretskii <eliz@gnu.org>, mvoteiza@udel.edu,
> >> emacs-devel@gnu.org, tino.calancha@gmail.com
> >> Date: Sat, 06 May 2017 19:51:33 +0200
> >> 
> >> > +  (if (string-match (concat "\\`" (or  regexp "[ \t\n\r]+")) string)
> >> 
> >> I think regexp should be surrounded by a grouping construct, like:
> >> 
> >>    (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
> >> 
> >> And similarly for string-trim-right.
> >
> > Rationale?
> 
> regexp can contain \|

Shouldn't such a regexp be surrounded by "\\(..\\)"?

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

[Johan Bockgård]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Johan Bockgård <bojohan@gnu.org>
>> Cc: mvoteiza@udel.edu, jean.christophe.helary@gmail.com,
>> tino.calancha@gmail.com, emacs-devel@gnu.org
>> Date: Sat, 06 May 2017 21:55:52 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Johan Bockgård <bojohan@gnu.org>
>> >> Cc: Eli Zaretskii <eliz@gnu.org>, mvoteiza@udel.edu,
>> >> emacs-devel@gnu.org, tino.calancha@gmail.com
>> >> Date: Sat, 06 May 2017 19:51:33 +0200
>> >> 
>> >> > +  (if (string-match (concat "\\`" (or  regexp "[ \t\n\r]+")) string)
>> >> 
>> >> I think regexp should be surrounded by a grouping construct, like:
>> >> 
>> >>    (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
>> >> 
>> >> And similarly for string-trim-right.
>> >
>> > Rationale?
>> 
>> regexp can contain \|
>
> Shouldn't such a regexp be surrounded by "\\(..\\)"?

This code performs string concatenation on a perfectly valid regexp
provided by the caller. IMO, it is this code's responsibility that the
result is sensible after the concatenation.

Are you saying this should be the caller's duty?

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

[Eli Zaretskii]

> From: Johan Bockgård <bojohan@gnu.org>
> Cc: mvoteiza@udel.edu,  jean.christophe.helary@gmail.com,  emacs-devel@gnu.org,  tino.calancha@gmail.com
> Date: Sun, 07 May 2017 18:23:56 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> >> I think regexp should be surrounded by a grouping construct, like:
> >> >> 
> >> >>    (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
> >> >> 
> >> >> And similarly for string-trim-right.
> >> >
> >> > Rationale?
> >> 
> >> regexp can contain \|
> >
> > Shouldn't such a regexp be surrounded by "\\(..\\)"?
> 
> This code performs string concatenation on a perfectly valid regexp
> provided by the caller. IMO, it is this code's responsibility that the
> result is sensible after the concatenation.
> 
> Are you saying this should be the caller's duty?

I'm _asking_ whether it should be the caller's duty.

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

[Yuri Khan]

On Sun, May 7, 2017 at 11:53 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> >> >> I think regexp should be surrounded by a grouping construct, like:
>> >> >>
>> >> >>    (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
>
> I'm _asking_ whether it should be the caller's duty.

The issue is not unique to regular expressions. It is a poster example
for C preprocessor macros.

    #define TWICE(x) 2 * x

vs

    #define TWICE(x) (2 * (x))

The top example places the duty of correctly parenthesizing the macro
argument and the whole invocation on the caller. The bottom example
assumes that duty itself.

Pretty much every coding style guide resolves the issue in favor of
the latter. Meyers’ razor: [Make interfaces easy to use correctly and
hard to use incorrectly.][1]

(Also, pretty much every C++ coding style guide says “avoid
preprocessor macros; use language constructs that actually care for
syntax, such as functions and function templates”. In Elisp, the
closest although imperfect analogy would probably be (rx-to-string `(:
string-start (regexp ,regexp))), which does add a non-capturing group
where needed.)

[1]: http://www.aristeia.com/Papers/IEEE_Software_JulAug_2004_revised.htm


Now, the issue at hand is: Do we want to expose all the broken code
that is using the existing interface incorrectly, or do we keep
backward bug compatibility?

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

[Jean-Christophe Helary]

> On May 10, 2017, at 18:21, Yuri Khan <yuri.v.khan@gmail.com> wrote:
> 
> Now, the issue at hand is: Do we want to expose all the broken code
> that is using the existing interface incorrectly, or do we keep
> backward bug compatibility?

No, the issue at hand is, should my modifications to subr-x.el conform to the bugged but seemingly standard approach, or use a safer approach, *before* we consider the issue you mention :)

Jean-Christophe

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

[Stefan Monnier]

>     #define TWICE(x) 2 * x
>
> vs
>
>     #define TWICE(x) (2 * (x))

Exactly.  And I can't believe we're discussing whether to keep the first
form or not.


        Stefan

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

[Jean-Christophe Helary]

> On May 10, 2017, at 20:56, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>>    #define TWICE(x) 2 * x
>> 
>> vs
>> 
>>    #define TWICE(x) (2 * (x))
> 
> Exactly.  And I can't believe we're discussing whether to keep the first
> form or not.

Ok, that settles it. Thank you.

Jean-Christophe

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

[Jean-Christophe Helary]

I'm sorry, my only use for groupings was for recording a regexp and referring to it later, so I pretty sure I don't understand what you discuss here. But I'd like to...

> On May 7, 2017, at 5:03, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>>>> I think regexp should be surrounded by a grouping construct, like:
>>>> 
>>>>   (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
>>>> 
>>>> And similarly for string-trim-right.
>>> 
>>> Rationale?
>> 
>> regexp can contain \|
> 
> Shouldn't such a regexp be surrounded by "\\(..\\)"?

So you both agree that the regexp should be surrounded by a grouping construct but you're arguing whether it should be a "normal" grouping or a shy grouping ?

Also,

>> This code performs string concatenation on a perfectly valid regexp
>> provided by the caller. IMO, it is this code's responsibility that the
>> result is sensible after the concatenation.
>> 
>> Are you saying this should be the caller's duty?
> 
> I'm _asking_ whether it should be the caller's duty.

How does that relate to the "shy" vs "non-shy" thing above?

Jean-Christophe 

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

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Mon, 8 May 2017 11:40:33 +0900
> Cc: Johan Bockgård <bojohan@gnu.org>,
>  mvoteiza@udel.edu,
>  tino.calancha@gmail.com,
>  emacs-devel@gnu.org
> 
> >>>> I think regexp should be surrounded by a grouping construct, like:
> >>>> 
> >>>>   (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
> >>>> 
> >>>> And similarly for string-trim-right.
> >>> 
> >>> Rationale?
> >> 
> >> regexp can contain \|
> > 
> > Shouldn't such a regexp be surrounded by "\\(..\\)"?
> 
> So you both agree that the regexp should be surrounded by a grouping construct but you're arguing whether it should be a "normal" grouping or a shy grouping ?

No, the issue is whether the function should enclose the argument in a
grouping constructs of any kind, or should this be the caller's
responsibility when the regexp includes \|.

> > I'm _asking_ whether it should be the caller's duty.
> 
> How does that relate to the "shy" vs "non-shy" thing above?

It doesn't.

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

[Jean-Christophe Helary]

> No, the issue is whether the function should enclose the argument in a
> grouping constructs of any kind, or should this be the caller's
> responsibility when the regexp includes \|.

(split-string) in subr.el (line 3808) doesn't use grouping constructs so why do something different than what's already the norm in an official emacs lisp file? Doesn't that settle the issue?

Jean-Christophe 

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

[Johan Bockgård]

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

>> No, the issue is whether the function should enclose the argument in a
>> grouping constructs of any kind, or should this be the caller's
>> responsibility when the regexp includes \|.
>
> (split-string) in subr.el (line 3808) doesn't use grouping constructs
> so why do something different than what's already the norm in an
> official emacs lisp file? Doesn't that settle the issue?

To me, that's nothing other than a bug:

    (split-string "abc, ;def g,hi" nil nil "[,;]")
    =>
      ("abc" "def" "g,hi")

    ;; Should be equivalent!
    (split-string "abc, ;def g,hi" nil nil ",\\|;")
    =>
      ("abc" "def" "g")  ; BUG

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

[Jean-Christophe Helary]

> On May 9, 2017, at 7:45, Johan Bockgård <bojohan@gnu.org> wrote:
> 
> Jean-Christophe Helary <jean.christophe.helary@gmail.com> writes:
> 
>>> No, the issue is whether the function should enclose the argument in a
>>> grouping constructs of any kind, or should this be the caller's
>>> responsibility when the regexp includes \|.
>> 
>> (split-string) in subr.el (line 3808) doesn't use grouping constructs
>> so why do something different than what's already the norm in an
>> official emacs lisp file? Doesn't that settle the issue?
> 
> To me, that's nothing other than a bug:
> 
>    (split-string "abc, ;def g,hi" nil nil "[,;]")
>    =>
>      ("abc" "def" "g,hi")
> 
>    ;; Should be equivalent!
>    (split-string "abc, ;def g,hi" nil nil ",\\|;")
>    =>
>      ("abc" "def" "g")  ; BUG

Ok, and the way to get your result is:

(split-string "abc, ;def g,hi" nil nil "\\(,\\|;\\)")

which means that Emacs thinks it is the caller's duty to provide the grouping construct, which also happens to be ugly, and non-intuitive since ",\\|;" is an otherwise valid regexp...

So, how do we go about that? Do we report a bug? Or do we find a good reason for that to happen and modify the documentation for split-string ?

Jean-Christophe

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

[Michael Heerdegen]

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

> >    (split-string "abc, ;def g,hi" nil nil ",\\|;")
> >    =>
> >      ("abc" "def" "g")  ; BUG
>
> Ok, and the way to get your result is:
>
> (split-string "abc, ;def g,hi" nil nil "\\(,\\|;\\)")
>
> which means that Emacs thinks it is the caller's duty to provide the
> grouping construct, which also happens to be ugly, and non-intuitive
> since ",\\|;" is an otherwise valid regexp...
>
> So, how do we go about that? Do we report a bug? Or do we find a good
> reason for that to happen and modify the documentation for
> split-string ?

I'm curious what could be reasons not to fix this (in the code).
Allowing only a subset of possible regexps would be quite strange.
Would there be any advantages?


Michael.

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

[Jean-Christophe Helary]

> On May 9, 2017, at 21:05, Michael Heerdegen <michael_heerdegen@web.de> wrote:
> 
> Jean-Christophe Helary <jean.christophe.helary@gmail.com> writes:
> 
>>>   (split-string "abc, ;def g,hi" nil nil ",\\|;")
>>>   =>
>>>     ("abc" "def" "g")  ; BUG
>> 
>> Ok, and the way to get your result is:
>> 
>> (split-string "abc, ;def g,hi" nil nil "\\(,\\|;\\)")
>> 
>> which means that Emacs thinks it is the caller's duty to provide the
>> grouping construct, which also happens to be ugly, and non-intuitive
>> since ",\\|;" is an otherwise valid regexp...
>> 
>> So, how do we go about that? Do we report a bug? Or do we find a good
>> reason for that to happen and modify the documentation for
>> split-string ?
> 
> I'm curious what could be reasons not to fix this (in the code).

Because split-string is used all over the place in Emacs and we'd have to check that the fix is not breaking some code somewhere in ways that we can't always guess?

> Allowing only a subset of possible regexps would be quite strange.

Not if you document the strangeness.

> Would there be any advantages?

Not having to modify a lot of other places in the Emacs code.

Jean-Christophe 

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

[Michael Heerdegen]

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

> > I'm curious what could be reasons not to fix this (in the code).
>
> Because split-string is used all over the place in Emacs and we'd have
> to check that the fix is not breaking some code somewhere in ways that
> we can't always guess?

I see.

OTOH we would probably also fix a lot of use cases that were broken
until now, unrecognized, in subtle ways.  Also in code outside of the
Emacs repository.

> > Allowing only a subset of possible regexps would be quite strange.
>
> Not if you document the strangeness.

Per definitionem.  In the long term, it might still be less work to fix
it.


Michael.

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

[Michael Heerdegen]

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

> Not if you document the strangeness.

BTW, if you really do this (I hope you don't), note that the strangeness
would also have to be documented for variables that are used at that
argument position of `split-string'.  An example is `crm-separator' in
crm.el.


Michael.

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

[Jean-Christophe Helary]

> On May 10, 2017, at 18:30, Michael Heerdegen <michael_heerdegen@web.de> wrote:
> 
> Jean-Christophe Helary <jean.christophe.helary@gmail.com> writes:
> 
>> Not if you document the strangeness.
> 
> BTW, if you really do this (I hope you don't), note that the strangeness
> would also have to be documented for variables that are used at that
> argument position of `split-string'.  An example is `crm-separator' in
> crm.el.

*I* won't :) But I'd expect somebody to report Johan's bug and the bug to be fixed in a way or another (either fix the documentation or fix the code). But that's something that's totally beyond what I'm able to do.

Jean-Christophe 

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

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Johan Bockgård <bojohan@gnu.org>,  mvoteiza@udel.edu,
>   Eli Zaretskii <eliz@gnu.org>,  tino.calancha@gmail.com,
>   emacs-devel@gnu.org
> Date: Tue, 09 May 2017 14:05:13 +0200
> 
> Jean-Christophe Helary <jean.christophe.helary@gmail.com> writes:
> 
> > >    (split-string "abc, ;def g,hi" nil nil ",\\|;")
> > >    =>
> > >      ("abc" "def" "g")  ; BUG
> >
> > Ok, and the way to get your result is:
> >
> > (split-string "abc, ;def g,hi" nil nil "\\(,\\|;\\)")
> >
> > which means that Emacs thinks it is the caller's duty to provide the
> > grouping construct, which also happens to be ugly, and non-intuitive
> > since ",\\|;" is an otherwise valid regexp...
> >
> > So, how do we go about that? Do we report a bug? Or do we find a good
> > reason for that to happen and modify the documentation for
> > split-string ?
> 
> I'm curious what could be reasons not to fix this (in the code).
> Allowing only a subset of possible regexps would be quite strange.
> Would there be any advantages?

How about if we proceed with the change in string-trim, and file a
separate bug against both it and split-string, for such regexps?
These seem to be largely unrelated issues, and it's a pity to block
Jean-Christophe's changes due to that.

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

[Jean-Christophe Helary]

> On May 10, 2017, at 0:23, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Michael Heerdegen <michael_heerdegen@web.de>
>> Cc: Johan Bockgård <bojohan@gnu.org>,  mvoteiza@udel.edu,
>>  Eli Zaretskii <eliz@gnu.org>,  tino.calancha@gmail.com,
>>  emacs-devel@gnu.org
>> Date: Tue, 09 May 2017 14:05:13 +0200
>> 
>> Jean-Christophe Helary <jean.christophe.helary@gmail.com> writes:
>> 
>>>>   (split-string "abc, ;def g,hi" nil nil ",\\|;")
>>>>   =>
>>>>     ("abc" "def" "g")  ; BUG
>>> 
>>> Ok, and the way to get your result is:
>>> 
>>> (split-string "abc, ;def g,hi" nil nil "\\(,\\|;\\)")
>>> 
>>> which means that Emacs thinks it is the caller's duty to provide the
>>> grouping construct, which also happens to be ugly, and non-intuitive
>>> since ",\\|;" is an otherwise valid regexp...
>>> 
>>> So, how do we go about that? Do we report a bug? Or do we find a good
>>> reason for that to happen and modify the documentation for
>>> split-string ?
>> 
>> I'm curious what could be reasons not to fix this (in the code).
>> Allowing only a subset of possible regexps would be quite strange.
>> Would there be any advantages?
> 
> How about if we proceed with the change in string-trim, and file a
> separate bug against both it and split-string, for such regexps?
> These seem to be largely unrelated issues, and it's a pity to block
> Jean-Christophe's changes due to that.

Ok, so we agree that we don't add \\( \\) to the regexp and that we expect the caller to send a correct regexp as is the case for split-string, is that correct ?

(But I'm  also glad the discussion that started with a trivial modification to ns-win.el triggered all that *and* that I understood what was at stake :-)

Jean-Christophe

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

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Wed, 10 May 2017 00:33:47 +0900
> Cc: Michael Heerdegen <michael_heerdegen@web.de>,
>  Johan Bockgård <bojohan@gnu.org>,
>  Mark Oteiza <mvoteiza@udel.edu>,
>  tino.calancha@gmail.com,
>  emacs-devel@gnu.org
> 
> > How about if we proceed with the change in string-trim, and file a
> > separate bug against both it and split-string, for such regexps?
> > These seem to be largely unrelated issues, and it's a pity to block
> > Jean-Christophe's changes due to that.
> 
> Ok, so we agree that we don't add \\( \\) to the regexp and that we expect the caller to send a correct regexp as is the case for split-string, is that correct ?

_I_ obviously agree, and so do you, but I'd like to hear from others
as well.

Thanks.

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

[Jean-Christophe Helary]

> On May 10, 2017, at 1:21, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>>> How about if we proceed with the change in string-trim, and file a
>>> separate bug against both it and split-string, for such regexps?
>>> These seem to be largely unrelated issues, and it's a pity to block
>>> Jean-Christophe's changes due to that.
>> 
>> Ok, so we agree that we don't add \\( \\) to the regexp and that we expect the caller to send a correct regexp as is the case for split-string, is that correct ?
> 
> _I_ obviously agree, and so do you, but I'd like to hear from others as well.

Johan, Stephan what's your take on this?

Since what I propose is consistant with the existing code, not agreeing with that means *first* finding the bug that Johan mentioned (which is not directly related to my modification), starting a review process of the whole code base that uses subr.el/split-string (and maybe other places where that regexp bug happens), fixing the code to ensure some kind of backward compatibility, etc. and *then* applying my change to subr-x...

While we could first change subr-x (string-trim...) so that they behave similarly to split-string and then start the bug hunting...

Jean-Christophe

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

[Stefan Monnier]

> Since what I propose is consistant with the existing code, not agreeing with
> that means *first* finding the bug that Johan mentioned (which is not
> directly related to my modification), starting a review process of the whole
> code base that uses subr.el/split-string (and maybe other places where that
> regexp bug happens), fixing the code to ensure some kind of backward
> compatibility, etc. and *then* applying my change to subr-x...

I'm pretty sure surrounding the regexp in \(?:..\) will break no code at
all.  To me it's an absolute no-brainer that doesn't even merit any discussion.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Johan Bockgård <bojohan@gnu.org>,  Michael
>  Heerdegen <michael_heerdegen@web.de>,  Eli Zaretskii <eliz@gnu.org>,  Mark
>  Oteiza <mvoteiza@udel.edu>,  Tino Calancha <tino.calancha@gmail.com>,
>   emacs-devel <emacs-devel@gnu.org>
> Date: Tue, 09 May 2017 20:45:14 -0400
> 
> I'm pretty sure surrounding the regexp in \(?:..\) will break no code at
> all.  To me it's an absolute no-brainer that doesn't even merit any discussion.

What about the same issue in split-string?

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

[Stefan Monnier]

>> I'm pretty sure surrounding the regexp in \(?:..\) will break no code
>> at all.  To me it's an absolute no-brainer that doesn't even merit
>> any discussion.

> What about the same issue in split-string?

Same thing.


        Stefan

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

[Jean-Christophe Helary]

> On May 10, 2017, at 9:45, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> Since what I propose is consistant with the existing code, not agreeing with
>> that means *first* finding the bug that Johan mentioned (which is not
>> directly related to my modification), starting a review process of the whole
>> code base that uses subr.el/split-string (and maybe other places where that
>> regexp bug happens), fixing the code to ensure some kind of backward
>> compatibility, etc. and *then* applying my change to subr-x...
> 
> I'm pretty sure surrounding the regexp in \(?:..\) will break no code at
> all.  To me it's an absolute no-brainer that doesn't even merit any discussion.

That does not seem to be the case. Johan has identified a case where a regexp that uses \| does not produce the same result as the "trim" argument of split-string whether it uses \( \) or not.

So there is the possibility that code that use the "trim" argument will produce an unexpected result if split-string is modified.

I am not talking about subr-x.el, it is an addition that is backward compatible since the new argument is optional.

Jean-Christophe 

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

[Stefan Monnier]

>> I'm pretty sure surrounding the regexp in \(?:..\) will break no code
>> at all.  To me it's an absolute no-brainer that doesn't even merit
>> any discussion.
> That does not seem to be the case.  Johan has identified a case
> where a regexp that uses \| does not produce the same result as the "trim"
> argument of split-string whether it uses \( \) or not.

Of course it doesn't behave the same (otherwise, why bother changing
it): "I'm pretty sure surrounding the regexp in \(?:..\) will break no
code at all" doesn't mean that such code can't exist, but that I expect
such code not to exist in the wild (or if it exists, it *suffers* from
the current behavior rather than benefitting from it).


        Stefan

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

[Andreas Schwab]

On Mai 10 2017, Jean-Christophe Helary <jean.christophe.helary@gmail.com> wrote:

> Ok, so we agree that we don't add \\( \\) to the regexp and that we expect the caller to send a correct regexp as is the case for split-string, is that correct ?

If you want to modify a regexp you need to make sure to preserve its
semantics.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Michael Heerdegen
>  <michael_heerdegen@web.de>,  Mark Oteiza <mvoteiza@udel.edu>,
>   emacs-devel@gnu.org,  Johan Bockgård <bojohan@gnu.org>,
>   tino.calancha@gmail.com
> Date: Tue, 09 May 2017 18:37:47 +0200
> 
> > Ok, so we agree that we don't add \\( \\) to the regexp and that we expect the caller to send a correct regexp as is the case for split-string, is that correct ?
> 
> If you want to modify a regexp you need to make sure to preserve its
> semantics.

Can you give an example of a regexp whose semantics might change by
adding \\(..\\)?

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

[Andreas Schwab]

On Mai 10 2017, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Andreas Schwab <schwab@suse.de>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  Michael Heerdegen
>>  <michael_heerdegen@web.de>,  Mark Oteiza <mvoteiza@udel.edu>,
>>   emacs-devel@gnu.org,  Johan Bockgård <bojohan@gnu.org>,
>>   tino.calancha@gmail.com
>> Date: Tue, 09 May 2017 18:37:47 +0200
>> 
>> > Ok, so we agree that we don't add \\( \\) to the regexp and that we expect the caller to send a correct regexp as is the case for split-string, is that correct ?
>> 
>> If you want to modify a regexp you need to make sure to preserve its
>> semantics.
>
> Can you give an example of a regexp whose semantics might change by
> adding \\(..\\)?

If you have any backrefs, their numbers change.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Jean-Christophe Helary]

> On May 10, 2017, at 16:45, Andreas Schwab <schwab@suse.de> wrote:
> 
>> Can you give an example of a regexp whose semantics might change by
>> adding \\(..\\)?
> 
> If you have any backrefs, their numbers change.

I think Eli meant using \(?:

Jean-Christophe 

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

[Jean-Christophe Helary]

Any conclusion on that point?

Jean-Christophe 

> On May 7, 2017, at 4:55, Johan Bockgård <bojohan@gnu.org> wrote:
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>>>> +  (if (string-match (concat "\\`" (or  regexp "[ \t\n\r]+")) string)
>>> 
>>> I think regexp should be surrounded by a grouping construct, like:
>>> 
>>>   (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
>>> 
>>> And similarly for string-trim-right.
>> 
>> Rationale?
> 
> regexp can contain \|

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

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Sun, 7 May 2017 13:39:43 +0900
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  mvoteiza@udel.edu,
>  tino.calancha@gmail.com,
>  emacs-devel@gnu.org
> 
> Any conclusion on that point?

Not yet.

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

[Stefan Monnier]

> I think regexp should be surrounded by a grouping construct, like:
>
>    (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+") "\\)")
>
> And similarly for string-trim-right.

Indeed, thanks,


        Stefan

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

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

Hopefully last fixes.

I've added the \\(?: ... \\) parens to make sure we capture any potential issue with \|.

Is there anything else I've missed ?

Jean-Christophe 


===========================
Add optional regexps for subr-x.el trimming functions

* lisp/emacs-lisp/subr-x.el (string-trim-left, string-trim-right, string-trim): add optional regexps that default on the original behavior.
===========================



[-- Attachment #2: subr-x.el.diff --]
[-- Type: application/octet-stream, Size: 1625 bytes --]

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 440213eb38..ded231c475 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -179,21 +179,27 @@ VARLIST can just be a plain tuple.
 
 (define-obsolete-function-alias 'string-reverse 'reverse "25.1")
 
-(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]+\"."
+  (if (string-match (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+")"\\)") string)
       (replace-match "" t t string)
     string))
 
-(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)
       (replace-match "" t t string)
     string))
 
-(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))
 
 (defsubst string-blank-p (string)
   "Check whether STRING is either empty or only whitespace."

[-- Attachment #3: Type: text/plain, Size: 1120 bytes --]





> On May 6, 2017, at 20:02, Jean-Christophe Helary <jean.christophe.helary@gmail.com> wrote:
> 
> Ok, I'm fine with that. Apologies for my lack of understanding of the issues.
> 
> Jean-Christophe
> 
>> On May 6, 2017, at 19:43, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>>> Date: Sat, 6 May 2017 19:33:10 +0900
>>> Cc: tino.calancha@gmail.com,
>>> mvoteiza@udel.edu,
>>> emacs-devel@gnu.org
>>> 
>>> My original idea was to put the regexp into a variable that *can* eventually be overridden by a user preference.
>> 
>> Does this really have legitimate use cases?  We are talking about a
>> very low-level function; overriding its default definition of
>> whitespace by a user option would affect every single use of this
>> function, and will most probably disrupt some code which uses it.
>> 
>> I think having a way for overriding the default programmatically
>> should be good enough, at least for now.  Let's not provide a
>> defcustom unless and until we have use cases for that which we want to
>> support.
> 

bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

Since nobody reacted in the last 3 days, I guess that means there is nothing to change, so I'm sending the patch here.

Jean-Christophe 

> Begin forwarded message:
> 
> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Subject: Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification
> Date: May 10, 2017 23:26:04 GMT+9
> To: Emacs developers <emacs-devel@gnu.org>
> 
> Hopefully last fixes.
> 
> I've added the \\(?: ... \\) parens to make sure we capture any potential issue with \|.
> 
> Is there anything else I've missed ?
> 
> Jean-Christophe 
> 
> 
> ===========================
> Add optional regexps for subr-x.el trimming functions
> 
> * lisp/emacs-lisp/subr-x.el (string-trim-left, string-trim-right, string-trim): add optional regexps that default on the original behavior.
> ===========================
> 
> 
> 

[-- Attachment #2.1: Type: text/html, Size: 3045 bytes --]

[-- Attachment #2.2: subr-x.el.diff --]
[-- Type: application/octet-stream, Size: 1625 bytes --]

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 440213eb38..ded231c475 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -179,21 +179,27 @@ VARLIST can just be a plain tuple.
 
 (define-obsolete-function-alias 'string-reverse 'reverse "25.1")
 
-(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]+\"."
+  (if (string-match (concat "\\`\\(?:" (or  regexp "[ \t\n\r]+")"\\)") string)
       (replace-match "" t t string)
     string))
 
-(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)
       (replace-match "" t t string)
     string))
 
-(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))
 
 (defsubst string-blank-p (string)
   "Check whether STRING is either empty or only whitespace."

[-- Attachment #2.3: Type: text/html, Size: 343 bytes --]

Re: bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 1281 bytes --]

Nobody is interested in checking/commiting that ? If it's something I should not have touched in the first place, please somebody tell me to drop it...

Jean-Christophe

> On May 13, 2017, at 22:36, Jean-Christophe Helary <jean.christophe.helary@gmail.com> wrote:
> 
> Since nobody reacted in the last 3 days, I guess that means there is nothing to change, so I'm sending the patch here.
> 
> Jean-Christophe 
> 
>> Begin forwarded message:
>> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com <mailto:jean.christophe.helary@gmail.com>>
>> Subject: Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification
>> Date: May 10, 2017 23:26:04 GMT+9
>> To: Emacs developers <emacs-devel@gnu.org <mailto:emacs-devel@gnu.org>>
>> 
>> Hopefully last fixes.
>> 
>> I've added the \\(?: ... \\) parens to make sure we capture any potential issue with \|.
>> 
>> Is there anything else I've missed ?
>> 
>> Jean-Christophe 
>> 
>> 
>> ===========================
>> Add optional regexps for subr-x.el trimming functions
>> 
>> * lisp/emacs-lisp/subr-x.el (string-trim-left, string-trim-right, string-trim): add optional regexps that default on the original behavior.
>> ===========================
>> 
>> 
> <subr-x.el.diff>
>> 


[-- Attachment #2: Type: text/html, Size: 4126 bytes --]

bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Wed, 17 May 2017 07:21:19 +0900
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> Nobody is interested in checking/commiting that ?

I usually wait for a week at least, to let people comment.  So please
be a bit more patient.

Thanks for working on this.

Re: bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Wed, 17 May 2017 07:21:19 +0900
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> Nobody is interested in checking/commiting that ?

I usually wait for a week at least, to let people comment.  So please
be a bit more patient.

Thanks for working on this.

bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Jean-Christophe Helary]

Thank you Eli. I was not aware of the procedure. I have no problem waiting :)

JC

> On May 17, 2017, at 11:26, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> Date: Wed, 17 May 2017 07:21:19 +0900
>> Cc: emacs-devel <emacs-devel@gnu.org>
>> 
>> Nobody is interested in checking/commiting that ?
> 
> I usually wait for a week at least, to let people comment.  So please
> be a bit more patient.
> 
> Thanks for working on this.

Re: bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Jean-Christophe Helary]

Thank you Eli. I was not aware of the procedure. I have no problem waiting :)

JC

> On May 17, 2017, at 11:26, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> Date: Wed, 17 May 2017 07:21:19 +0900
>> Cc: emacs-devel <emacs-devel@gnu.org>
>> 
>> Nobody is interested in checking/commiting that ?
> 
> I usually wait for a week at least, to let people comment.  So please
> be a bit more patient.
> 
> Thanks for working on this.

bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 1281 bytes --]

Nobody is interested in checking/commiting that ? If it's something I should not have touched in the first place, please somebody tell me to drop it...

Jean-Christophe

> On May 13, 2017, at 22:36, Jean-Christophe Helary <jean.christophe.helary@gmail.com> wrote:
> 
> Since nobody reacted in the last 3 days, I guess that means there is nothing to change, so I'm sending the patch here.
> 
> Jean-Christophe 
> 
>> Begin forwarded message:
>> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com <mailto:jean.christophe.helary@gmail.com>>
>> Subject: Re: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification
>> Date: May 10, 2017 23:26:04 GMT+9
>> To: Emacs developers <emacs-devel@gnu.org <mailto:emacs-devel@gnu.org>>
>> 
>> Hopefully last fixes.
>> 
>> I've added the \\(?: ... \\) parens to make sure we capture any potential issue with \|.
>> 
>> Is there anything else I've missed ?
>> 
>> Jean-Christophe 
>> 
>> 
>> ===========================
>> Add optional regexps for subr-x.el trimming functions
>> 
>> * lisp/emacs-lisp/subr-x.el (string-trim-left, string-trim-right, string-trim): add optional regexps that default on the original behavior.
>> ===========================
>> 
>> 
> <subr-x.el.diff>
>> 


[-- Attachment #2: Type: text/html, Size: 4126 bytes --]

bug#26908: Fwd: Trimming strings, /emacs/lisp/emacs-lisp/subr-x.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Sat, 13 May 2017 22:36:11 +0900
> 
> Since nobody reacted in the last 3 days, I guess that means there is nothing to change, so I'm sending the patch here.

Thanks, pushed.

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

[Jean-Christophe Helary]

>> My original idea was to put the regexp into a variable that *can* eventually be overridden by a user preference.
> 
> Does this really have legitimate use cases?  We are talking about a
> very low-level function; overriding its default definition of
> whitespace by a user option would affect every single use of this
> function, and will most probably disrupt some code which uses it.

By the way, the idea I had was like what we have in Applescript for the split function.
AS provides the system with a default list of splitters and the user can locally change it for specific purposes and then revert it to it's original value.

In AS, this is used very frequently.

Jean-Christophe 

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

[Drew Adams]

> > +(defsubst string-trim-left (string &optional regexp)
> > +  "Trim STRING of leading whitespace matching REGEXP.
> > +
> > +REGEXP defaults to \"[ \t\n\r]+\"."
> > +  (if (string-match (concat "\\`" (or regexp "[ \t\n\r]+")) string)
> >       (replace-match "" t t string)
> >     string))
>
> Isn't there a way to do [...] that would work with defsubst?

IMHO, *none* of the defsubsts in subr-x.el should be there.
They should all be defuns.  (Currently, even their names are
inconsistent, some with `--' to indicate internal, most not.)

The code will be byte-compiled.  I see zero reason why these
functions should be defsubsts.  Instead of using a boatload
of defsubsts willy nilly, any defsubsts we put in the code
should be _strongly_ justified.  I don't see any justification
for these defsubsts.

It's not just because `defsubst' still exists that we should
use it casually (or even use it at all).  It is a precambrian
vestige, with little or no real use case.

Just one opinion.

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

[Stefan Monnier]

> The code will be byte-compiled.  I see zero reason why these
> functions should be defsubsts.

The reason, AFAIC is the following: subr-x holds functions which we
haven't committed to adding to core.  We exceptionally allow them to
not use a prefix but in return we only use subr-x while compiling (since
it's fundamentally not namespace-clean and we don't want to impose those
non-clean names upon the unsuspecting user).


        Stefan

subr-x.el code and defsubst [was: Trimming strings, /.../subr-x.el modification]

[Drew Adams]

> > The code will be byte-compiled.  I see zero reason why these
> > functions should be defsubsts.
> 
> The reason, AFAIC is the following: subr-x holds functions which we
> haven't committed to adding to core.  We exceptionally allow them to
> not use a prefix but in return we only use subr-x while compiling (since
> it's fundamentally not namespace-clean and we don't want to impose those
> non-clean names upon the unsuspecting user).

OK.  Not a great reason, IMO, but I understand now, at least.

I don't think that reason is obvious.  Please consider adding a
GIANT comment in subr-x.el to explain this exceptional treatment.

This existing comment, which is all there is, does not cover it, IMO:

;; Less commonly used functions that complement basic APIs, often
;; implemented in C code (like hash-tables and strings), and are
;; not eligible for inclusion in subr.el.

;; Do not document these functions in the lispref.
;; http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01006.html

And why is it that this library has still not been "committed to
core"?  (And isn't that what GNU ELPA is for?)

Beyond that, this approach of not bothering with name prefixes,
byte-compiling separately, etc. seems really weird (fragile, error
prone).  Why such an exception?  Is this really the best way to
handle code that you consider "experimental" or "less commonly used"
or code "that complements basic APIs"?

Re: subr-x.el code and defsubst [was: Trimming strings, /.../subr-x.el modification]

[Tianxiang Xiong]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

I also don't understand why `subr-x.el` exists.

First of all, why not put `string-*` functions in a `string.el`? It's
surprising that no such file exists at the moment.

Secondly, why are these functions considered "experimental"? If a package
depends on a function in `subr-x` that is later removed, is it any less
inconvenient for the package author than if `subr-x` was *not* experimental?
No. Empirically, the `string-*` and `thread-*` functions see a lot of use
in the wild, so (re)moving them at this point would cause quite an uproar.

If functions need to considered experimental, at least put them in the
"right" place--e.g. `string-empty-p` in `string.el`. That way, even if the
function's implementation needed to be modified, package authors won't need
to `require` a different package. Organization matters.


On Sat, May 6, 2017 at 11:07 AM, Drew Adams <drew.adams@oracle.com> wrote:

> > > The code will be byte-compiled.  I see zero reason why these
> > > functions should be defsubsts.
> >
> > The reason, AFAIC is the following: subr-x holds functions which we
> > haven't committed to adding to core.  We exceptionally allow them to
> > not use a prefix but in return we only use subr-x while compiling (since
> > it's fundamentally not namespace-clean and we don't want to impose those
> > non-clean names upon the unsuspecting user).
>
> OK.  Not a great reason, IMO, but I understand now, at least.
>
> I don't think that reason is obvious.  Please consider adding a
> GIANT comment in subr-x.el to explain this exceptional treatment.
>
> This existing comment, which is all there is, does not cover it, IMO:
>
> ;; Less commonly used functions that complement basic APIs, often
> ;; implemented in C code (like hash-tables and strings), and are
> ;; not eligible for inclusion in subr.el.
>
> ;; Do not document these functions in the lispref.
> ;; http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01006.html
>
> And why is it that this library has still not been "committed to
> core"?  (And isn't that what GNU ELPA is for?)
>
> Beyond that, this approach of not bothering with name prefixes,
> byte-compiling separately, etc. seems really weird (fragile, error
> prone).  Why such an exception?  Is this really the best way to
> handle code that you consider "experimental" or "less commonly used"
> or code "that complements basic APIs"?
>
>

[-- Attachment #2: Type: text/html, Size: 3129 bytes --]

RE: subr-x.el code and defsubst [was: Trimming strings, /.../subr-x.el modification]

[Drew Adams]

> I also don't understand why `subr-x.el` exists. 
>
> First of all, why not put `string-*` functions in a `string.el`?
> It's surprising that no such file exists at the moment.
>
> Secondly, why are these functions considered "experimental"?
> If a package depends on a function in `subr-x` that is later
> removed, is it any less inconvenient for the package author
> than if `subr-x` was not experimental? No. Empirically, the
> `string-*` and `thread-*` functions see a lot of use in the
> wild, so (re)moving them at this point would cause quite an
> uproar.
>
> If functions need to considered experimental, at least put
> them in the "right" place--e.g. `string-empty-p` in `string.el`.
> That way, even if the function's implementation needed to
> be modified, package authors won't need to `require` a
> different package. Organization matters.

FWIW, I agree.

Perhaps it's not a coincidence that this policy/approach was
accompanied in time by the introduction (or perhaps increase)
of the use of "*--*" names for "internal" Lisp constructs.
Similarly misguided, I think.

If you really feel the need for a "purgatory" or "lab" for
code that you don't yet feel confident about or haven't yet
consecrated as official or backed by commitment, code that
you consider experimental or internal so that you can feel
more freedom to modify it without worrying about users, at
least stick it in GNU ELPA, please.

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

[Andreas Schwab]

On Mai 06 2017, Tino Calancha <tino.calancha@gmail.com> wrote:

> +(defsubst string-trim-left (string &optional regexp)
> +  "Trim STRING of leading whitespace matching REGEXP.
> +
> +REGEXP defaults to \"[ \t\n\r]+\"."

You need to quote the backslashes in the string.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Tino Calancha]

On Sat, 6 May 2017, Andreas Schwab wrote:

> On Mai 06 2017, Tino Calancha <tino.calancha@gmail.com> wrote:
>
>> +(defsubst string-trim-left (string &optional regexp)
>> +  "Trim STRING of leading whitespace matching REGEXP.
>> +
>> +REGEXP defaults to \"[ \t\n\r]+\"."
>
> You need to quote the backslashes in the string.
>
> Andreas.
Indeed. Thank you!
Tino