GNU bug report logs - #16078
Extensive docs and tests for `ruby-forward-string' (PATCH)

Previous Next

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

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

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

From: Cameron Desautels <camdez <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Extensive docs and tests for `ruby-forward-string' (PATCH)
Date: Fri, 6 Dec 2013 11:15:52 -0600

[Message part 1 (text/plain, inline)]

The attached patch adds extensive documentation and tests to the
`ruby-forward-string' function.

This may seem an odd function to document thoroughly, but I spent
quite a while wrapping my head around the exact behavior and I want to
spare the next person.  It also underlies some important parsing
functionality in ruby-mode.

Note the one test which is expected to fail: this represents an
outstanding bug in `ruby-forward-string`.  I'll be (immediately)
following this report with a patch which fixes *that* issue, but it
didn't seem prudent to combine the commits.
--
Cameron Desautels <camdez <at> gmail.com>

[ruby-forward-string-docs-and-tests.diff (text/plain, attachment)]

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Cameron Desautels <camdez <at> gmail.com>
Cc: 16078 <at> debbugs.gnu.org
Subject: Re: bug#16078: Extensive docs and tests for `ruby-forward-string'
 (PATCH)
Date: Sat, 07 Dec 2013 05:22:55 +0200

Hi Cameron,

Cameron Desautels <camdez <at> gmail.com> writes:

> The attached patch adds extensive documentation and tests to the
> `ruby-forward-string' function.
>
> This may seem an odd function to document thoroughly, but I spent
> quite a while wrapping my head around the exact behavior and I want to
> spare the next person.  It also underlies some important parsing
> functionality in ruby-mode.

Thank you for your effort, but it probably would've been more valuable a
few months ago or earlier.  The ruby-mode that will be released with
Emacs 24.4 has switched to using SMIE for indentation and sexp
navigation by default, and it leaves quite a bit of the old,
undocumented code unused.

Whatever code examples and functionality didn't work for you, have you
tried them with the current trunk?

We can still use the two patches, of course, since the old indentation
engine can still be enabled with `(setq ruby-use-smie nil)', but they
exceed the 15 line limit and will require copyright assignment (AFAIR,
we do require those even for test code).  Have you signed, or are you
willing to sign the copyright assignment papers?

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

From: Cameron Desautels <camdez <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 16078 <at> debbugs.gnu.org
Subject: Re: bug#16078: Extensive docs and tests for `ruby-forward-string'
 (PATCH)
Date: Sat, 7 Dec 2013 20:29:15 -0600

Hi Dmitry,

Thank you for explaining the recent changes.  I did see a lot of
duplicated parsing code and wasn't completely sure what the reason for
it was.  I read up a bit on SMIE so that makes sense now.

However, I've done additional testing on the current trunk and the
issue persists.

As a minimal example to recreate the issue, create a new file with the
following contents (indentation removed):

    def foo
      %^bar^
    end

Turn on ruby-mode, and run `imenu-add-menubar-index'.  You will get
the following error:

    Error in menu-bar-update-hook (imenu-update-menubar):
(invalid-regexp "Unmatched [ or [^")

(Note that if you run it again it will appear to work--run `imenu' to
see the error again.)  This stems from the bug in
`ruby-forward-string' that my patch fixes.

I have not signed the copyright assignment papers but I am willing.

Thanks.
--
Cameron Desautels <camdez <at> gmail.com>


On Fri, Dec 6, 2013 at 9:22 PM, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> Hi Cameron,
>
> Cameron Desautels <camdez <at> gmail.com> writes:
>
>> The attached patch adds extensive documentation and tests to the
>> `ruby-forward-string' function.
>>
>> This may seem an odd function to document thoroughly, but I spent
>> quite a while wrapping my head around the exact behavior and I want to
>> spare the next person.  It also underlies some important parsing
>> functionality in ruby-mode.
>
> Thank you for your effort, but it probably would've been more valuable a
> few months ago or earlier.  The ruby-mode that will be released with
> Emacs 24.4 has switched to using SMIE for indentation and sexp
> navigation by default, and it leaves quite a bit of the old,
> undocumented code unused.
>
> Whatever code examples and functionality didn't work for you, have you
> tried them with the current trunk?
>
> We can still use the two patches, of course, since the old indentation
> engine can still be enabled with `(setq ruby-use-smie nil)', but they
> exceed the 15 line limit and will require copyright assignment (AFAIR,
> we do require those even for test code).  Have you signed, or are you
> willing to sign the copyright assignment papers?

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Cameron Desautels <camdez <at> gmail.com>
Cc: 16078 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>
Subject: Re: bug#16078: Extensive docs and tests for `ruby-forward-string'
 (PATCH)
Date: Mon, 09 Dec 2013 06:07:44 +0200

On 08.12.2013 04:29, Cameron Desautels wrote:
> Thank you for explaining the recent changes.  I did see a lot of
> duplicated parsing code and wasn't completely sure what the reason for
> it was.  I read up a bit on SMIE so that makes sense now.

Even without SMIE, there already was duplication between 
`ruby-syntax-propertize-function' (and its handling of string 
interpolations and percent literals) and the manual parsing of them in 
`ruby-parse-partial' and `ruby-forward-string'.

So, if I were going to untangle that code, first I'd have tried to 
replace all the uses of `ruby-forward-string' with `forward-sexp', and 
maybe `narrow-to-region', to handle the END parameter.  We already know 
where those literals end, no need to read them char-by-char again.

> As a minimal example to recreate the issue, create a new file with the
> following contents (indentation removed):
>
>      def foo
>        %^bar^
>      end

Thank you for the example.

> Turn on ruby-mode, and run `imenu-add-menubar-index'.  You will get
> the following error:

I've now also fixed it not to call `ruby-parse-partial' when SMIE is used.

This leaves `ruby-move-to-block' as the only function that still 
requires the old indentation engine to work. Not sure yet how to rewrite 
it in terms of sexp navigation or other SMIE-specific functions.

> I have not signed the copyright assignment papers but I am willing.

For now I've installed everything except the tests. Combined with your 
patch for #16046, the docstring and the patch in #16079 go over the 15 
lines limit, though not far.

If the more experienced contributors say it's too much, we can remove 
the docstring.

For the tests, though, and any further patches, the assignment will be 
required.

Glenn, could you send the form?

Speaking of the tests, though, the way they are now, they only cover one 
parsing/indentation engine. If they were written to be more high-level, 
say, testing indentation or the IMenu index generation, they could touch 
both engines, which would be better.

Although if `ruby-forward-string' were replaced with `forward-sexp', 
there probably would be no need for most of the new tests, since the 
code would handle all cases uniformly.

N.B.: Your example above also uncovered an indentation bug when using 
SMIE. Gonna fix that shortly.

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

From: Cameron Desautels <camdez <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 16078 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>
Subject: Re: bug#16078: Extensive docs and tests for `ruby-forward-string'
 (PATCH)
Date: Wed, 11 Dec 2013 16:48:58 -0600

[Message part 1 (text/plain, inline)]

On Sun, Dec 8, 2013 at 10:07 PM, Dmitry Gutov <dgutov <at> yandex.ru> wrote:

> Speaking of the tests, though, the way they are now, they only cover one
> parsing/indentation engine. If they were written to be more high-level,
> say, testing indentation or the IMenu index generation, they could touch
> both engines, which would be better.


I agree completely. But I didn't have my head around how all of the code
worked and I wanted to contribute as I could. Plus I though it would be
best to include some tests for my work, even if they weren't as high level
as would be ideal.

Let me know what else you need from me.

-- 
Cameron Desautels <camdez <at> gmail.com>

[Message part 2 (text/html, inline)]

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Cameron Desautels <camdez <at> gmail.com>
Cc: 16078 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>
Subject: Re: bug#16078: Extensive docs and tests for `ruby-forward-string'
 (PATCH)
Date: Sun, 15 Dec 2013 05:36:25 +0200

On 12.12.2013 00:48, Cameron Desautels wrote:
> I agree completely. But I didn't have my head around how all of the code
> worked and I wanted to contribute as I could. Plus I though it would be
> best to include some tests for my work, even if they weren't as high
> level as would be ideal.
>
> Let me know what else you need from me.

I'm sure someone will send you the papers to sign, sooner or later.

In the meantime, you could rewrite the tests to be more high-level, or 
work on replacing the uses of `ruby-forward-string' with `forward-sexp' 
(like I described earlier) if you have the time. If not, that's okay too.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Cameron Desautels <camdez <at> gmail.com>
Cc: 16078-done <at> debbugs.gnu.org
Subject: Re: bug#16078: Extensive docs and tests for `ruby-forward-string'
 (PATCH)
Date: Sun, 15 Nov 2015 16:55:09 +0200

Cameron,

Nothing has happened here for a while, so I'm closing this bug.

Thanks again for the contribution, even if we only accepted a part of it.

Previous Next

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