Re: [nongnu] elpa/helm 07dacfe2e2 08/11: Prefer string-match-p over string-suffix-p

Re: [nongnu] elpa/helm 07dacfe2e2 08/11: Prefer string-match-p over string-suffix-p

[Philip Kaludercic]

ELPA Syncer <elpasync@gnu.org> writes:

> branch: elpa/helm
> commit 07dacfe2e2db980a9e42afef2fc8539c155fdd0d
> Author: Thierry Volpiatto <thievol@posteo.net>
> Commit: Thierry Volpiatto <thievol@posteo.net>
>
>     Prefer string-match-p over string-suffix-p
> ---
>  helm-lib.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/helm-lib.el b/helm-lib.el
> index 2a93271f57..70df22089c 100644
> --- a/helm-lib.el
> +++ b/helm-lib.el
> @@ -1732,7 +1732,7 @@ Directories expansion is not supported."
>                    (with-temp-buffer
>                      (call-process-shell-command 
>                       (format cmd
> -                             (if (string-suffix-p ".gz" file)
> +                             (if (string-match-p ".gz\\'" file)

Is there a reason for this preference?  Also, I assume you want to quote
the period in ".gz\\'"?  If so, I can recommend using `rx' or
`wildcard-to-regexp' to avoid mistakes like these.

>                                   "gzip -c -q -d" "cat")
>                               (shell-quote-argument file)
>                               regexp)

Re: [nongnu] elpa/helm 07dacfe2e2 08/11: Prefer string-match-p over string-suffix-p

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Thierry Volpiatto <thievol@posteo.net>
> Date: Thu, 14 Sep 2023 12:25:55 +0000
> 
> ELPA Syncer <elpasync@gnu.org> writes:
> 
> > --- a/helm-lib.el
> > +++ b/helm-lib.el
> > @@ -1732,7 +1732,7 @@ Directories expansion is not supported."
> >                    (with-temp-buffer
> >                      (call-process-shell-command 
> >                       (format cmd
> > -                             (if (string-suffix-p ".gz" file)
> > +                             (if (string-match-p ".gz\\'" file)
> 
> Is there a reason for this preference?

I think this change is simply incorrect: the first argument of
string-suffix-p is not interpreted as a regexp, but as a simple
literal string (the implementation uses compare-strings internally).

> Also, I assume you want to quote the period in ".gz\\'"?

No need to quote it, see above.

Re: [nongnu] elpa/helm 07dacfe2e2 08/11: Prefer string-match-p over string-suffix-p

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Thierry Volpiatto <thievol@posteo.net>
>> Date: Thu, 14 Sep 2023 12:25:55 +0000
>> 
>> ELPA Syncer <elpasync@gnu.org> writes:
>> 
>> > --- a/helm-lib.el
>> > +++ b/helm-lib.el
>> > @@ -1732,7 +1732,7 @@ Directories expansion is not supported."
>> >                    (with-temp-buffer
>> >                      (call-process-shell-command 
>> >                       (format cmd
>> > -                             (if (string-suffix-p ".gz" file)
>> > +                             (if (string-match-p ".gz\\'" file)
>> 
>> Is there a reason for this preference?
>
> I think this change is simply incorrect: the first argument of
> string-suffix-p is not interpreted as a regexp, but as a simple
> literal string (the implementation uses compare-strings internally).

But why is it incorrect?  `string-suffix-p' is passed a string, while
`string-match-p' takes a regular expression, that might be too liberal
but should still match everything the previous check did -- or am I
missing something?

>> Also, I assume you want to quote the period in ".gz\\'"?
>
> No need to quote it, see above.

Re: [nongnu] elpa/helm 07dacfe2e2 08/11: Prefer string-match-p over string-suffix-p

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: emacs-devel@gnu.org,  thievol@posteo.net
> Date: Thu, 14 Sep 2023 13:06:47 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think this change is simply incorrect: the first argument of
> > string-suffix-p is not interpreted as a regexp, but as a simple
> > literal string (the implementation uses compare-strings internally).
> 
> But why is it incorrect?  `string-suffix-p' is passed a string, while
> `string-match-p' takes a regular expression, that might be too liberal
> but should still match everything the previous check did -- or am I
> missing something?

Why do the change when string-suffix-p already ensures there's nothing
in the second argument after the suffix?

Re: [nongnu] elpa/helm 07dacfe2e2 08/11: Prefer string-match-p over string-suffix-p

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: emacs-devel@gnu.org,  thievol@posteo.net
>> Date: Thu, 14 Sep 2023 13:06:47 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I think this change is simply incorrect: the first argument of
>> > string-suffix-p is not interpreted as a regexp, but as a simple
>> > literal string (the implementation uses compare-strings internally).
>> 
>> But why is it incorrect?  `string-suffix-p' is passed a string, while
>> `string-match-p' takes a regular expression, that might be too liberal
>> but should still match everything the previous check did -- or am I
>> missing something?
>
> Why do the change when string-suffix-p already ensures there's nothing
> in the second argument after the suffix?

I don't know, that is what I wanted to ask Thierry.

Re: [nongnu] elpa/helm 07dacfe2e2 08/11: Prefer string-match-p over string-suffix-p

[Thierry Volpiatto]

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


Philip Kaludercic <philipk@posteo.net> writes:

> ELPA Syncer <elpasync@gnu.org> writes:
>
>> branch: elpa/helm
>> commit 07dacfe2e2db980a9e42afef2fc8539c155fdd0d
>> Author: Thierry Volpiatto <thievol@posteo.net>
>> Commit: Thierry Volpiatto <thievol@posteo.net>
>>
>>     Prefer string-match-p over string-suffix-p
>> ---
>>  helm-lib.el | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/helm-lib.el b/helm-lib.el
>> index 2a93271f57..70df22089c 100644
>> --- a/helm-lib.el
>> +++ b/helm-lib.el
>> @@ -1732,7 +1732,7 @@ Directories expansion is not supported."
>>                    (with-temp-buffer
>>                      (call-process-shell-command 
>>                       (format cmd
>> -                             (if (string-suffix-p ".gz" file)
>> +                             (if (string-match-p ".gz\\'" file)
>
> Is there a reason for this preference?

Just a preference, I tried string-suffix-p but I prefer string-match-p
which is more readable, for me at least.

> Also, I assume you want to quote the period in ".gz\\'"?

Yes, my mistake.

> If so, I can recommend using `rx' or `wildcard-to-regexp' to avoid
> mistakes like these.

No thanks, I hate rx and unneeded complications more generally, I am used
to plain regexps.

>
>>                                   "gzip -c -q -d" "cat")
>>                               (shell-quote-argument file)
>>                               regexp)


-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]