bug#31800: suggestion of improvement for sort-numeric-fields function.

bug#31800: suggestion of improvement for sort-numeric-fields function.

[SK Kim]

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

This is not likely a bug but sort-numeric-fields function does not allow
region with blank lines, while sort-lines does.

This was because sort-skip-fields occurs error with blank line. And when I
added condition for sort-skip-fields like below, sort-numeric-fields was
working with blank lines too.

                     (when (not (string-match-p "^\\s-*$" (thing-at-point
'line)))
                       (sort-skip-fields field))

So, I just hope sort-numeric-fields would work for region with blank lines
too.

I tested with GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.18.9) of 2018-05-29


Always appreciate for contribution.
Thanks.

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

bug#31800: suggestion of improvement for sort-numeric-fields function.

[Lars Ingebrigtsen]

SK Kim <tttuuu888@gmail.com> writes:

> This is not likely a bug but sort-numeric-fields function does not
> allow region with blank lines, while sort-lines does.
>
> This was because sort-skip-fields occurs error with blank line. And
> when I added condition for sort-skip-fields like below,
> sort-numeric-fields was working with blank lines too.
>
>                      (when (not (string-match-p "^\\s-*$" (thing-at-point 'line)))
>                        (sort-skip-fields field))
>
> So, I just hope sort-numeric-fields would work for region with blank
> lines too.

Can you create a patch for this fix?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#31800: suggestion of improvement for sort-numeric-fields function.

[SK Kim]

[-- Attachment #1.1: Type: text/plain, Size: 967 bytes --]

I made this patch from github emacs-mirror repository.
I hope this will be of help.

Thanks.

2019년 7월 13일 (토) 오후 12:42, Lars Ingebrigtsen <larsi@gnus.org>님이 작성:

> SK Kim <tttuuu888@gmail.com> writes:
>
> > This is not likely a bug but sort-numeric-fields function does not
> > allow region with blank lines, while sort-lines does.
> >
> > This was because sort-skip-fields occurs error with blank line. And
> > when I added condition for sort-skip-fields like below,
> > sort-numeric-fields was working with blank lines too.
> >
> >                      (when (not (string-match-p "^\\s-*$"
> (thing-at-point 'line)))
> >                        (sort-skip-fields field))
> >
> > So, I just hope sort-numeric-fields would work for region with blank
> > lines too.
>
> Can you create a patch for this fix?
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

[-- Attachment #1.2: Type: text/html, Size: 1782 bytes --]

[-- Attachment #2: 0001-Improve-sort-numeric-fields.patch --]
[-- Type: text/x-patch, Size: 908 bytes --]

From d08c5d368337ee49bb72d9915c409edcbc73b4e0 Mon Sep 17 00:00:00 2001
From: SeungKi Kim <tttuuu888@gmail.com>
Date: Sat, 13 Jul 2019 17:12:46 +0900
Subject: [PATCH] Improve `sort-numeric-fields'

* lisp/sort.el (sort-numeric-fields) : Allow including empty lines.
---
 lisp/sort.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/sort.el b/lisp/sort.el
index 6ea1c44060..3e9413e4af 100644
--- a/lisp/sort.el
+++ b/lisp/sort.el
@@ -281,7 +281,8 @@ FIELD, BEG and END.  BEG and END specify region to sort."
       ((inhibit-field-text-motion t))
     (sort-fields-1 field beg end
 		   (lambda ()
-		     (sort-skip-fields field)
+                     (unless (string-match-p "^\\s-*$" (thing-at-point 'line))
+                       (sort-skip-fields field))
 		     (let* ((case-fold-search t)
 			    (base
 			     (if (looking-at "\\(0x\\)[0-9a-f]\\|\\(0\\)[0-7]")
-- 
2.22.0

bug#31800: suggestion of improvement for sort-numeric-fields function.

[Stefan Kangas]

SK Kim <tttuuu888@gmail.com> writes:

> I made this patch from github emacs-mirror repository.
> I hope this will be of help.
>
> Thanks.
>
> 2019년 7월 13일 (토) 오후 12:42, Lars Ingebrigtsen <larsi@gnus.org>님이 작성:
>
>  SK Kim <tttuuu888@gmail.com> writes:
>
>  > This is not likely a bug but sort-numeric-fields function does not
>  > allow region with blank lines, while sort-lines does.
>  >
>  > This was because sort-skip-fields occurs error with blank line. And
>  > when I added condition for sort-skip-fields like below,
>  > sort-numeric-fields was working with blank lines too.
>  >
>  >                      (when (not (string-match-p "^\\s-*$" (thing-at-point 'line)))
>  >                        (sort-skip-fields field))
>  >
>  > So, I just hope sort-numeric-fields would work for region with blank
>  > lines too.

The patch below looks good to me.  Does anyone else have an opinion
here, or should I go ahead and commit it to master?

Best regards,
Stefan Kangas

> From d08c5d368337ee49bb72d9915c409edcbc73b4e0 Mon Sep 17 00:00:00 2001
> From: SeungKi Kim <tttuuu888@gmail.com>
> Date: Sat, 13 Jul 2019 17:12:46 +0900
> Subject: [PATCH] Improve `sort-numeric-fields'
>
> * lisp/sort.el (sort-numeric-fields) : Allow including empty lines.
> ---
>  lisp/sort.el | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/sort.el b/lisp/sort.el
> index 6ea1c44060..3e9413e4af 100644
> --- a/lisp/sort.el
> +++ b/lisp/sort.el
> @@ -281,7 +281,8 @@ FIELD, BEG and END.  BEG and END specify region to sort."
>        ((inhibit-field-text-motion t))
>      (sort-fields-1 field beg end
>  		   (lambda ()
> -		     (sort-skip-fields field)
> +                     (unless (string-match-p "^\\s-*$" (thing-at-point 'line))
> +                       (sort-skip-fields field))
>  		     (let* ((case-fold-search t)
>  			    (base
>  			     (if (looking-at "\\(0x\\)[0-9a-f]\\|\\(0\\)[0-7]")

bug#31800: suggestion of improvement for sort-numeric-fields function.

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> The patch below looks good to me.  Does anyone else have an opinion
> here, or should I go ahead and commit it to master?

[...]

>> -		     (sort-skip-fields field)
>> +                     (unless (string-match-p "^\\s-*$" (thing-at-point 'line))
>> +                       (sort-skip-fields field))

I don't think this patch makes sense as is -- I think that's a
convoluted way of saying `looking-at'?  But it makes conceptual sense, I
think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#31800: suggestion of improvement for sort-numeric-fields function.

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>> -		     (sort-skip-fields field)
>>> +                     (unless (string-match-p "^\\s-*$" (thing-at-point 'line))
>>> +                       (sort-skip-fields field))
>
> I don't think this patch makes sense as is -- I think that's a
> convoluted way of saying `looking-at'?  But it makes conceptual sense, I
> think.

I'm not sure.  Do you mean to use:

    (looking-at "\\s*$")

Are we sure point is at the beginning of line here though?  The above
code doesn't require that, but also seems to be wrong if region is in
the middle of a line.  But I'm not sure how people typically use this
in that case.

BTW, shouldn't we also fix the same bug in sort-fields while we're at
it?

Best regards,
Stefan Kangas

bug#31800: suggestion of improvement for sort-numeric-fields function.

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>>>> -		     (sort-skip-fields field)
>>>> + (unless (string-match-p "^\\s-*$" (thing-at-point 'line))
>>>> +                       (sort-skip-fields field))
>>
>> I don't think this patch makes sense as is -- I think that's a
>> convoluted way of saying `looking-at'?  But it makes conceptual sense, I
>> think.
>
> I'm not sure.  Do you mean to use:
>
>     (looking-at "\\s*$")
>
> Are we sure point is at the beginning of line here though?  The above
> code doesn't require that, but also seems to be wrong if region is in
> the middle of a line.  But I'm not sure how people typically use this
> in that case.

I assumed that

  (thing-at-point 'line)

returns the current line, but I haven't actually looked at the code, but
you're right -- we don't know where point is, so that has to be moved
before checking.  In any case, using thing-at-point is not the right
thing.

> BTW, shouldn't we also fix the same bug in sort-fields while we're at
> it?

Yes.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#31800: suggestion of improvement for sort-numeric-fields function.

[Lars Ingebrigtsen]

SK Kim <tttuuu888@gmail.com> writes:

> This is not likely a bug but sort-numeric-fields function does not allow region with
> blank lines, while sort-lines does.
>
> This was because sort-skip-fields occurs error with blank line. And
> when I added condition for sort-skip-fields like below,
> sort-numeric-fields was working with blank lines too.

I've now fixed this in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no