bug#21072: Brave new mark-defun (and a testing tool)

classic Classic list List threaded Threaded
64 messages Options
1234
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3

On 2017-02-13, at 20:00, John Wiegley <[hidden email]> wrote:

>>>>>> Dmitry Gutov <[hidden email]> writes:
>
>> I'd rather interpret John as being entirely serious. :) Tests are good.
>
> Dmitry is quite right; any patch that comes with a battery of new tests is
> already a huge plus in my book.

Thanks - as I said, I was a bit unsure;-).

Here's my proposed contribution, formatted as two patches.  The first
one introduces the testing machinery; the second one introduces
mark-defun and its tests.

WDYT?

--
Marcin Borkowski

0001-Add-elisp-tests-with-temp-buffer-a-new-testing-macro.patch (2K) Download Attachment
0002-Fix-bug-21072-and-rework-mark-defun.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Dmitry Gutov
On 14.02.2017 12:45, Marcin Borkowski wrote:
> +(defun in-comment-line-p ()

This needs a different name.

Something like beginning-of-defun--in-comment-line-p might be a good choice.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3

On 2017-02-14, at 14:02, Dmitry Gutov <[hidden email]> wrote:

> On 14.02.2017 12:45, Marcin Borkowski wrote:
>> +(defun in-comment-line-p ()
>
> This needs a different name.
>
> Something like beginning-of-defun--in-comment-line-p might be a good choice.

Why?  It seems to me that it may be of general use.

Best,

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3

On 2017-02-14, at 20:25, Stefan Monnier <[hidden email]> wrote:

>> Why?  It seems to me that it may be of general use.
>
> If you want it to be general, it'll have to be better defined.
> What is a "comment line"?

A line containing only a comment (possibly after whitespace).  I guess
the docstring could benefit from explaining this.

Thanks,

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Stefan Monnier
>>> Why?  It seems to me that it may be of general use.
>> If you want it to be general, it'll have to be better defined.
>> What is a "comment line"?
> A line containing only a comment (possibly after whitespace).

Is a line (using C syntax) like:

    /* blablabla

considered as a "comment line"?
What about the likely next line:

    blablabla */

?
How about

    blablabla

on a line between the previous two (i.e. within a comment)?

Regardless of the answer you give above, I'm wondering in which kind of
circumstance we'd want to test if we're on "a line containing only
a comment".


        Stefan



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3

On 2017-02-15, at 08:56, Stefan Monnier <[hidden email]> wrote:

>>>> Why?  It seems to me that it may be of general use.
>>> If you want it to be general, it'll have to be better defined.
>>> What is a "comment line"?
>> A line containing only a comment (possibly after whitespace).
>
> Is a line (using C syntax) like:
>
>     /* blablabla
>
> considered as a "comment line"?

Yes.

> What about the likely next line:
>
>     blablabla */

Yes.

>
> ?
> How about
>
>     blablabla
>
> on a line between the previous two (i.e. within a comment)?

Yes.

(However, I found a minor bug: an empty line, even between a line "/*"
and another with "*/" is _not_ considered a comment line by my
function.  I'll try to fix it.

> Regardless of the answer you give above, I'm wondering in which kind of
> circumstance we'd want to test if we're on "a line containing only
> a comment".

You will be surprised, then, that I actually did use a very similar
function in completely another circumstance: a command that counts
source lines of code in a region, and excludes lines containing only
whitespace, comments and docstrings.  (Never mind the discussion about
whether SLOC is meaningful in any sense;-).)

Best,

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Stefan Monnier
>> Regardless of the answer you give above, I'm wondering in which kind of
>> circumstance we'd want to test if we're on "a line containing only
>> a comment".
> You will be surprised, then, that I actually did use a very similar
> function in completely another circumstance: a command that counts
> source lines of code in a region, and excludes lines containing only
> whitespace, comments and docstrings.  (Never mind the discussion about
> whether SLOC is meaningful in any sense;-).)

My point is that it's not very frequent to need this exact definition of
a "comment line" and that there are various other possible definitions
one might need in other circumstances.
So at the very least, the doc should clarify which definition of
"comment line" it uses.


        Stefan



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3

On 2017-02-15, at 20:27, Stefan Monnier <[hidden email]> wrote:

>>> Regardless of the answer you give above, I'm wondering in which kind of
>>> circumstance we'd want to test if we're on "a line containing only
>>> a comment".
>> You will be surprised, then, that I actually did use a very similar
>> function in completely another circumstance: a command that counts
>> source lines of code in a region, and excludes lines containing only
>> whitespace, comments and docstrings.  (Never mind the discussion about
>> whether SLOC is meaningful in any sense;-).)
>
> My point is that it's not very frequent to need this exact definition of
> a "comment line" and that there are various other possible definitions
> one might need in other circumstances.
> So at the very least, the doc should clarify which definition of
> "comment line" it uses.

Understood.  Do you have then any better idea for the name of this
function?  beginning-of-defun--incomment-line-p seems to specific,
in-comment-line-p _may_ be indeed too general.  (I'll make the docstring
more precise, of course.)

Thank you all for looking at the patch,

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Stefan Monnier
> Understood.  Do you have then any better idea for the name of this
> function?  beginning-of-defun--incomment-line-p seems to specific,
> in-comment-line-p _may_ be indeed too general.

I'll let someone else decide if it deserves a "non-prefixed" name, but
as for the name after the potential prefix, I think focusing on
"comment" is the wrong idea.  Maybe `insignificant-line-p`?  Or `emptyish-line-p`?


        Stefan



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3

On 2017-02-16, at 14:22, Stefan Monnier <[hidden email]> wrote:

>> Understood.  Do you have then any better idea for the name of this
>> function?  beginning-of-defun--incomment-line-p seems to specific,
>> in-comment-line-p _may_ be indeed too general.
>
> I'll let someone else decide if it deserves a "non-prefixed" name, but
> as for the name after the potential prefix, I think focusing on
> "comment" is the wrong idea.  Maybe `insignificant-line-p`?  Or `emptyish-line-p`?

OK, so I have renamed it and expanded the docstring.  I attach
a corrected patch (the second one, the first one is the same as before).

Is there anything else I can do before we may apply this patch and
consider bug#21072 fixed?

(Notice that three places could be still corrected: two when bug#24427
is fixed and possibly another one when the strange behavior of
(beginning-of-defun 0) is fixed - I will officially file a bug about it
later.  But these apparently will have to wait.)

Best,

--
Marcin Borkowski

0002-Fix-bug-21072-and-rework-mark-defun.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Eli Zaretskii
> From: Marcin Borkowski <[hidden email]>
> Date: Fri, 17 Feb 2017 09:54:51 +0100
> Cc: [hidden email], [hidden email]
>
> OK, so I have renamed it and expanded the docstring.  I attach
> a corrected patch (the second one, the first one is the same as before).
>
> Is there anything else I can do before we may apply this patch and
> consider bug#21072 fixed?

Thanks, this looks good, but please move the tests to lisp-tests.el,
to keep our conventions wrt test names.

Then this could go in.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Dmitry Gutov
In reply to this post by Marcin Borkowski-3
On 17.02.2017 10:54, Marcin Borkowski wrote:
> +(defun in-emptyish-line-p ()

In case you were wondering, I'm still not sure this is a valuable
addition to our public API.

But if Eli says it's okay, then it's probably okay.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Eli Zaretskii
> From: Dmitry Gutov <[hidden email]>
> Date: Tue, 7 Mar 2017 18:50:33 +0200
> Cc: [hidden email], [hidden email]
>
> On 17.02.2017 10:54, Marcin Borkowski wrote:
> > +(defun in-emptyish-line-p ()
>
> In case you were wondering, I'm still not sure this is a valuable
> addition to our public API.
>
> But if Eli says it's okay, then it's probably okay.

I don't really have an opinion, but perhaps it would be better to make
it an internal function for now, indeed.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3
In reply to this post by Dmitry Gutov

On 2017-03-07, at 17:50, Dmitry Gutov <[hidden email]> wrote:

> On 17.02.2017 10:54, Marcin Borkowski wrote:
>> +(defun in-emptyish-line-p ()
>
> In case you were wondering, I'm still not sure this is a valuable
> addition to our public API.

Well, I renamed it anyway.

Thanks,

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3
In reply to this post by Eli Zaretskii

On 2017-03-07, at 17:53, Eli Zaretskii <[hidden email]> wrote:

>> From: Dmitry Gutov <[hidden email]>
>> Date: Tue, 7 Mar 2017 18:50:33 +0200
>> Cc: [hidden email], [hidden email]
>>
>> On 17.02.2017 10:54, Marcin Borkowski wrote:
>> > +(defun in-emptyish-line-p ()
>>
>> In case you were wondering, I'm still not sure this is a valuable
>> addition to our public API.
>>
>> But if Eli says it's okay, then it's probably okay.
>
> I don't really have an opinion, but perhaps it would be better to make
> it an internal function for now, indeed.

Yep, that's what I did.

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3
In reply to this post by Eli Zaretskii

On 2017-03-07, at 17:46, Eli Zaretskii <[hidden email]> wrote:

>> From: Marcin Borkowski <[hidden email]>
>> Date: Fri, 17 Feb 2017 09:54:51 +0100
>> Cc: [hidden email], [hidden email]
>>
>> OK, so I have renamed it and expanded the docstring.  I attach
>> a corrected patch (the second one, the first one is the same as before).
>>
>> Is there anything else I can do before we may apply this patch and
>> consider bug#21072 fixed?
>
> Thanks, this looks good, but please move the tests to lisp-tests.el,
> to keep our conventions wrt test names.
>
> Then this could go in.

I just pushed it to a branch, forgetting about this email.  I'll fix it
and report back.  For now, I deleted the branch I pushed; I'll pish it
again as soon as I correct this.

Thanks,

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3

On 2017-03-29, at 08:34, Marcin Borkowski <[hidden email]> wrote:

> On 2017-03-07, at 17:46, Eli Zaretskii <[hidden email]> wrote:
>
>>> From: Marcin Borkowski <[hidden email]>
>>> Date: Fri, 17 Feb 2017 09:54:51 +0100
>>> Cc: [hidden email], [hidden email]
>>>
>>> OK, so I have renamed it and expanded the docstring.  I attach
>>> a corrected patch (the second one, the first one is the same as before).
>>>
>>> Is there anything else I can do before we may apply this patch and
>>> consider bug#21072 fixed?
>>
>> Thanks, this looks good, but please move the tests to lisp-tests.el,
>> to keep our conventions wrt test names.
>>
>> Then this could go in.
>
> I just pushed it to a branch, forgetting about this email.  I'll fix it
> and report back.  For now, I deleted the branch I pushed; I'll pish it
> again as soon as I correct this.

OK, I pushed the branch "fix-bug-21072".  Can anyone confirm that it's
ok and either merge it into master or tell me that I can do it?

Best,

--
Marcin Borkowski



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Glenn Morris-3
Marcin Borkowski wrote:

> OK, I pushed the branch "fix-bug-21072".  Can anyone confirm that it's
> ok and either merge it into master or tell me that I can do it?

Nitpick: branch should have been called fix/bug-21072
(given lack of response to http://debbugs.gnu.org/25610, I can't blame you)

While I'm nitpicking, please don't cc emacs-devel on bug reports.

:)



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

npostavs
In reply to this post by Marcin Borkowski-3
Marcin Borkowski <[hidden email]> writes:

>
> OK, I pushed the branch "fix-bug-21072".  Can anyone confirm that it's
> ok and either merge it into master or tell me that I can do it?

>  
> +** New macro 'elisp-tests-with-temp-buffer'
> +which helps writing tests for functions that should change buffers in
> +specific ways or manipulate point or mark positions.
> +
> +---

I don't this should be documented in NEWS since the macro is being added
to a test file, so it's not part of Emacs' libraries.  Also, the format
of the NEWS entry is wrong in the same way as the next one (see below).

> +With a prefix argument, it marks that many defuns or extends the
> +region by the appropriate number of defuns.  With negative prefix
> +argument it marks defuns in the opposite direction and also changes
> +the direction of selecting for subsequent uses of @code{mark-defun}.

This doesn't say what exactly happens with zero as argument.  The code
seems to do something odd.  Perhaps it should just be a user-error
instead?  Or maybe just a nop.

> modified   etc/NEWS
> @@ -363,6 +363,15 @@ words where first character is upper rather than title case, e.g.,
>  "DŽungla" instead of "Džungla".
>  
>  
> +** New behavior of 'mark-defun' implemented
> +Prefix argument selects that many (or that many more) defuns.
> +Negative prefix arg flips the direction of selection.  Also,
> +'mark-defun' between defuns correctly selects N following defuns (or
> +-N previous for negative arguments).  Finally, comments preceding the
> +defun are selected unless they are separated from the defun by a blank
> +line.
> +
> ++++
> * Changes in Specialized Modes and Packages in Emacs 26.1
>

This entry should go before the page separator, and the "+++" should go
on the line just above the entry, not after it.

> +(defun beginning-of-defun-comments (&optional arg)

> +  (let (nbobp)
> +    (while (progn
> +             (setq nbobp (zerop (forward-line -1)))
> +             (and (not (looking-at "^\\s-*$"))
> +                  (beginning-of-defun--in-emptyish-line-p)
> +                  nbobp)))
> +    (when nbobp
> +      (forward-line 1))))


The looking-at call is redundant, right?  Anyway, can't that all be
replaced by just

    (forward-comment (- (point)))
    (unless (bolp)
      (forward-line 1))

> +(defun mark-defun (&optional arg)

> +  (let (nbobp)
> +    (while (progn
> +             (setq nbobp (zerop (forward-line -1)))
> +             (and (looking-at "^\\s-*$")
> +                  nbobp)))
> +    (when nbobp
> +      (forward-line 1))))

I think this can be just

    (skip-chars-backward "[:space:]\n")
    (unless (bolp)
      (forward-line 1))



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#21072: Brave new mark-defun (and a testing tool)

Marcin Borkowski-3
Hey,

and thanks for your feedback!

My answers to particular points are below.


On 2017-04-03, at 00:56, [hidden email] wrote:

> Marcin Borkowski <[hidden email]> writes:
>
>>
>> OK, I pushed the branch "fix-bug-21072".  Can anyone confirm that it's
>> ok and either merge it into master or tell me that I can do it?
>
>>  
>> +** New macro 'elisp-tests-with-temp-buffer'
>> +which helps writing tests for functions that should change buffers in
>> +specific ways or manipulate point or mark positions.
>> +
>> +---
>
> I don't this should be documented in NEWS since the macro is being added
> to a test file, so it's not part of Emacs' libraries.  Also, the format
> of the NEWS entry is wrong in the same way as the next one (see below).

I deleted that from etc/NEWS.

>> +With a prefix argument, it marks that many defuns or extends the
>> +region by the appropriate number of defuns.  With negative prefix
>> +argument it marks defuns in the opposite direction and also changes
>> +the direction of selecting for subsequent uses of @code{mark-defun}.
>
> This doesn't say what exactly happens with zero as argument.  The code
> seems to do something odd.  Perhaps it should just be a user-error
> instead?  Or maybe just a nop.

Good catch.  I guess a no-op is fine.

>> modified   etc/NEWS
>> @@ -363,6 +363,15 @@ words where first character is upper rather than title case, e.g.,
>>  "DŽungla" instead of "Džungla".
>>
>>  
>> +** New behavior of 'mark-defun' implemented
>> +Prefix argument selects that many (or that many more) defuns.
>> +Negative prefix arg flips the direction of selection.  Also,
>> +'mark-defun' between defuns correctly selects N following defuns (or
>> +-N previous for negative arguments).  Finally, comments preceding the
>> +defun are selected unless they are separated from the defun by a blank
>> +line.
>> +
>> ++++
>> * Changes in Specialized Modes and Packages in Emacs 26.1
>>
>
> This entry should go before the page separator, and the "+++" should go
> on the line just above the entry, not after it.

That one I do not understand.  This means that "+++" goes essentially
_to the previous entry_, which doesn't seem to make sense (especially
when viewing NEWS folded, which I assume everyone does, right?).

>> +(defun beginning-of-defun-comments (&optional arg)
>
>> +  (let (nbobp)
>> +    (while (progn
>> +             (setq nbobp (zerop (forward-line -1)))
>> +             (and (not (looking-at "^\\s-*$"))
>> +                  (beginning-of-defun--in-emptyish-line-p)
>> +                  nbobp)))
>> +    (when nbobp
>> +      (forward-line 1))))
>
>
> The looking-at call is redundant, right?  Anyway, can't that all be

Hm.  Probably yes, although this seems to be not very well documented in
`forward-comment's docs.

> replaced by just
>
>     (forward-comment (- (point)))
>     (unless (bolp)
>       (forward-line 1))

My tests say no.  Consider these contents of a buffer:

--8<---------------cut here---------------start------------->8---
;; Comment at the bob

(defun func (arg)
  "docstring"
  body)
--8<---------------cut here---------------end--------------->8---

Put the point inside the defun and call mark-defun.  Your version marks
the comment at the beginning, mine doesn't.

>> +(defun mark-defun (&optional arg)
>
>> +  (let (nbobp)
>> +    (while (progn
>> +             (setq nbobp (zerop (forward-line -1)))
>> +             (and (looking-at "^\\s-*$")
>> +                  nbobp)))
>> +    (when nbobp
>> +      (forward-line 1))))
>
> I think this can be just
>
>     (skip-chars-backward "[:space:]\n")
>     (unless (bolp)
>       (forward-line 1))

This OTOH does pass my tests, though I guess it would be clearer to
replace (bolp) with (bobp) in the above code (if I understand correctly,
in this situation they should be equivalent).  WDYT?

Thanks a lot,

--
Marcin Borkowski



1234
Loading...