GNU bug report logs - #60999
30.0.50; [PATCH] Add support for negative indices and index ranges in Eshell

Previous Next

Package: emacs;

Reported by: Jim Porter <jporterbugs <at> gmail.com>

Date: Sun, 22 Jan 2023 03:48:02 UTC

Severity: normal

Tags: patch

Found in version 30.0.50

Done: Jim Porter <jporterbugs <at> gmail.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60999 in the body.
You can then email your comments to 60999 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#60999; Package emacs. (Sun, 22 Jan 2023 03:48:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Jim Porter <jporterbugs <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sun, 22 Jan 2023 03:48:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Add support for negative indices and index ranges in
 Eshell
Date: Sat, 21 Jan 2023 19:47:47 -0800
[Message part 1 (text/plain, inline)]
There are two suggestions in the "Bugs and ideas" section of the Eshell 
manual:

  Allow "$_[-1]", which would indicate the last element of the array

  Make "$x[*]" equal to listing out the full contents of "x"

I think these would be pretty useful, especially for the "$_" variable, 
which gets the last argument of the last command, but if you give it an 
index like "$_[N]", gets the Nth argument of the last command. However, 
it's not as easy to get the second-to-last argument of the last command, 
or to get *all* arguments of the last command. So the above two 
suggestions would be pretty helpful.

Attached is a patch to do this. For the second suggestion, I took some 
liberties and added range syntax, so that "$x[2..5]" returns elements 2, 
3, and 4 (zero-indexed) of x.

I have just one question though: this implementation treats ranges as 
half-open, i.e. "M..N" is [M, N). I think that's the best way of doing 
things (and it matches how 'seq-subseq' works). However, "M..N" is the 
Bash syntax, which uses a closed range, [M, N]. Maybe this would be too 
confusing for users? I'm open to using other tokens aside from ".." if 
that would help. Maybe "M:N" would work? That's the Python syntax, which 
behaves the same way as this patch. Any thoughts?
[0001-Add-support-for-negative-indices-and-index-ranges-in.patch (text/plain, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60999; Package emacs. (Sun, 22 Jan 2023 06:30:02 GMT) Full text and rfc822 format available.

Message #8 received at 60999 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60999 <at> debbugs.gnu.org
Subject: Re: bug#60999: 30.0.50;
 [PATCH] Add support for negative indices and index ranges in Eshell
Date: Sun, 22 Jan 2023 08:29:20 +0200
> Date: Sat, 21 Jan 2023 19:47:47 -0800
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> Attached is a patch to do this. For the second suggestion, I took some 
> liberties and added range syntax, so that "$x[2..5]" returns elements 2, 
> 3, and 4 (zero-indexed) of x.
> 
> I have just one question though: this implementation treats ranges as 
> half-open, i.e. "M..N" is [M, N). I think that's the best way of doing 
> things (and it matches how 'seq-subseq' works). However, "M..N" is the 
> Bash syntax, which uses a closed range, [M, N]. Maybe this would be too 
> confusing for users? I'm open to using other tokens aside from ".." if 
> that would help. Maybe "M:N" would work? That's the Python syntax, which 
> behaves the same way as this patch. Any thoughts?

I think compatibility to other shells is an advantage.

> +(defcustom eshell-integer-regexp (rx (? "-") (+ digit))
> +  "Regular expression used to match integer arguments."
> +  :type 'regexp)

Why is this a defcustom? what alternative could a user possibly want?

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60999; Package emacs. (Sun, 22 Jan 2023 08:14:02 GMT) Full text and rfc822 format available.

Message #11 received at 60999 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60999 <at> debbugs.gnu.org
Subject: Re: bug#60999: 30.0.50; [PATCH] Add support for negative indices and
 index ranges in Eshell
Date: Sun, 22 Jan 2023 00:13:16 -0800
On 1/21/2023 10:29 PM, Eli Zaretskii wrote:
>> Date: Sat, 21 Jan 2023 19:47:47 -0800
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> I have just one question though: this implementation treats ranges as
>> half-open, i.e. "M..N" is [M, N). I think that's the best way of doing
>> things (and it matches how 'seq-subseq' works). However, "M..N" is the
>> Bash syntax, which uses a closed range, [M, N]. Maybe this would be too
>> confusing for users? [snip]
> 
> I think compatibility to other shells is an advantage.

True. Though, in practice I find that the half-open range is easier to 
use whenever things become more complex than just using constant values. 
For example, truncating a larger list to be the same size as a shorter 
one ("$#" means "length of"):

  $long-list[..$#short-list]

Using Bash-style closed ranges, you'd have to do:

  $long-list[..${1- $#short-list}]

(Note that unlike Bash syntax, I made this so that M and N are optional. 
That's another thing we could consider changing.)

Half-open ranges are also more consistent with Emacs Lisp in my 
experience. It would be strange (at least to me) to have things like 
'seq-subseq', 'add-text-properties', 'substring', etc use one way of 
specifying ranges, and Eshell to use another, especially when you can 
call any of those functions within Eshell. But like you say, 
compatibility with other shells is an advantage too...

In that sense, some non-Bash syntax might be nice. However, Eshell 
subscripts are already fairly complex, so we'd need to be careful about 
what we choose so that we don't break compatibility. For example, when 
subscripting a string, the first element in the subscript can be a 
regexp that splits the string, so to get the first element in your PATH, 
you could do:

  $PATH[: 0]

If ":" was the range separator, this would change meaning. There are 
ways to prevent that incompatible change if we're careful, but it's 
something to keep in mind. ".." has the advantage that while it's a 
valid regexp, it's not very useful in this context, so we probably 
wouldn't break anything in practice.

>> +(defcustom eshell-integer-regexp (rx (? "-") (+ digit))
>> +  "Regular expression used to match integer arguments."
>> +  :type 'regexp)
> 
> Why is this a defcustom? what alternative could a user possibly want?

Just for symmetry with 'eshell-number-regexp'. Maybe that shouldn't be a 
defcustom either though; if someone wanted to change 
'eshell-number-regexp', that's probably a sign that there's a bug, and 
the default value should be improved.

Either way we decide about 'eshell-number-regexp', I can turn 
'eshell-integer-regexp' into a regular defvar. (The only thing I can 
think of that a person would customize it to would be to allow a "+" at 
the start of an integer, like "+123".)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60999; Package emacs. (Fri, 27 Jan 2023 01:24:02 GMT) Full text and rfc822 format available.

Message #14 received at 60999 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60999 <at> debbugs.gnu.org
Subject: Re: bug#60999: 30.0.50; [PATCH] Add support for negative indices and
 index ranges in Eshell
Date: Thu, 26 Jan 2023 17:23:19 -0800
[Message part 1 (text/plain, inline)]
On 1/22/2023 12:13 AM, Jim Porter wrote:
> Either way we decide about 'eshell-number-regexp', I can turn 
> 'eshell-integer-regexp' into a regular defvar. (The only thing I can 
> think of that a person would customize it to would be to allow a "+" at 
> the start of an integer, like "+123".)

Ok, I've updated my patch to that 'eshell-integer-regexp' is just a 
regular defvar. In the second patch, I also converted 
'eshell-number-regexp' to a defvar, and improved the regexp to match 
more valid numbers. I think with those improvements, there's no real 
reason for 'eshell-number-regexp' to be customizable anymore.

Note: I haven't done anything with the range syntax though. If you feel 
strongly that it should be a closed range like in Bash (instead of 
half-open like it is in the current patch), then I don't mind changing 
it. Personally though, I have a soft preference for half-open since it's 
more consistent with the rest of Emacs Lisp.
[0001-Add-support-for-negative-indices-and-index-ranges-in.patch (text/plain, attachment)]
[0002-Make-eshell-number-regexp-into-a-regular-defvar.patch (text/plain, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60999; Package emacs. (Fri, 27 Jan 2023 07:39:02 GMT) Full text and rfc822 format available.

Message #17 received at 60999 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60999 <at> debbugs.gnu.org
Subject: Re: bug#60999: 30.0.50; [PATCH] Add support for negative indices and
 index ranges in Eshell
Date: Fri, 27 Jan 2023 09:38:11 +0200
> Date: Thu, 26 Jan 2023 17:23:19 -0800
> From: Jim Porter <jporterbugs <at> gmail.com>
> Cc: 60999 <at> debbugs.gnu.org
> 
> On 1/22/2023 12:13 AM, Jim Porter wrote:
> > Either way we decide about 'eshell-number-regexp', I can turn 
> > 'eshell-integer-regexp' into a regular defvar. (The only thing I can 
> > think of that a person would customize it to would be to allow a "+" at 
> > the start of an integer, like "+123".)
> 
> Ok, I've updated my patch to that 'eshell-integer-regexp' is just a 
> regular defvar. In the second patch, I also converted 
> 'eshell-number-regexp' to a defvar, and improved the regexp to match 
> more valid numbers. I think with those improvements, there's no real 
> reason for 'eshell-number-regexp' to be customizable anymore.

Thanks.

> Note: I haven't done anything with the range syntax though. If you feel 
> strongly that it should be a closed range like in Bash (instead of 
> half-open like it is in the current patch), then I don't mind changing 
> it. Personally though, I have a soft preference for half-open since it's 
> more consistent with the rest of Emacs Lisp.

I have no strong feelings, as I don't expect to be using this feature.
I just think this could be confusing to people who do use it in other
shells, and a potential source of complaints and bug reports.  But it
is your call.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60999; Package emacs. (Fri, 27 Jan 2023 18:03:01 GMT) Full text and rfc822 format available.

Message #20 received at 60999 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60999 <at> debbugs.gnu.org
Subject: Re: bug#60999: 30.0.50; [PATCH] Add support for negative indices and
 index ranges in Eshell
Date: Fri, 27 Jan 2023 10:02:30 -0800
On 1/26/2023 11:38 PM, Eli Zaretskii wrote:
> I have no strong feelings, as I don't expect to be using this feature.
> I just think this could be confusing to people who do use it in other
> shells, and a potential source of complaints and bug reports.  But it
> is your call.

Ok, thanks. Would it make sense to merge this patch as-is then, and 
afterwards I can post a message on emacs-devel soliciting feedback? If 
people clearly prefer the Bash semantics, I don't have any major issues 
with changing the code to match that.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#60999; Package emacs. (Fri, 27 Jan 2023 18:58:01 GMT) Full text and rfc822 format available.

Message #23 received at 60999 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 60999 <at> debbugs.gnu.org
Subject: Re: bug#60999: 30.0.50; [PATCH] Add support for negative indices and
 index ranges in Eshell
Date: Fri, 27 Jan 2023 20:57:16 +0200
> Date: Fri, 27 Jan 2023 10:02:30 -0800
> Cc: 60999 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> On 1/26/2023 11:38 PM, Eli Zaretskii wrote:
> > I have no strong feelings, as I don't expect to be using this feature.
> > I just think this could be confusing to people who do use it in other
> > shells, and a potential source of complaints and bug reports.  But it
> > is your call.
> 
> Ok, thanks. Would it make sense to merge this patch as-is then, and 
> afterwards I can post a message on emacs-devel soliciting feedback?

Feel free.




Reply sent to Jim Porter <jporterbugs <at> gmail.com>:
You have taken responsibility. (Sat, 28 Jan 2023 02:16:01 GMT) Full text and rfc822 format available.

Notification sent to Jim Porter <jporterbugs <at> gmail.com>:
bug acknowledged by developer. (Sat, 28 Jan 2023 02:16:02 GMT) Full text and rfc822 format available.

Message #28 received at 60999-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60999-done <at> debbugs.gnu.org
Subject: Re: bug#60999: 30.0.50; [PATCH] Add support for negative indices and
 index ranges in Eshell
Date: Fri, 27 Jan 2023 18:15:28 -0800
On 1/27/2023 10:57 AM, Eli Zaretskii wrote:
> Feel free.

Thanks. Merged as 5642bf0b97. I'll send a message to emacs-devel over 
the weekend.

Closing this now.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 25 Feb 2023 12:24:19 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 32 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.