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

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

[Cameron Desautels]

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

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@gmail.com>

[-- Attachment #2: ruby-forward-string-docs-and-tests.diff --]
[-- Type: text/plain, Size: 4148 bytes --]

*** lisp/progmodes/ruby-mode.el.orig	2013-12-06 10:56:52.000000000 -0600
--- lisp/progmodes/ruby-mode.el     	2013-12-06 10:54:07.000000000 -0600
***************
*** 791,797 ****
                    (t nil)))))))))

  (defun ruby-forward-string (term &optional end no-error expand)
!   "TODO: document."
    (let ((n 1) (c (string-to-char term))
          (re (if expand
                  (concat "[^\\]\\(\\\\\\\\\\)*\\([" term "]\\|\\(#{\\)\\)")
--- 791,811 ----
                    (t nil)))))))))

  (defun ruby-forward-string (term &optional end no-error expand)
!   "Move forward across one balanced pair of string delimiters.
! Skips escaped delimiters. If EXPAND is non-nil, also ignores
! delimiters in interpolated strings.
!
! TERM should be a string containing either a single, self-matching
! delimiter (e.g. \"/\"), or a pair of matching delimiters with the
! close delimiter first (e.g. \"][\").
!
! When non-nil, search is bounded by position END.
!
! Throws an error if a balanced match is not found, unless NO-ERROR
! is non-nil, in which case nil will be returned.
!
! This command assumes the character after point is an opening
! delimiter."
    (let ((n 1) (c (string-to-char term))
          (re (if expand
                  (concat "[^\\]\\(\\\\\\\\\\)*\\([" term "]\\|\\(#{\\)\\)")
*** test/automated/ruby-mode-tests.el.orig	2013-12-06 10:56:52.000000000 -0600
--- test/automated/ruby-mode-tests.el     	2013-12-06 10:54:07.000000000 -0600
***************
*** 639,644 ****
--- 639,695 ----
        (ruby--insert-coding-comment "utf-8")
        (should (string= "# encoding: utf-8\n\n" (buffer-string))))))

+ (defun ruby-forward-string-should-move-to (content term index &optional expand)
+   "Assert that `ruby-forward-string', called on buffer containing
+ CONTENT, passing TERM and EXPAND, leaves point at INDEX.
+
+ Pass nil for INDEX if an error should be expected."
+   (ruby-with-temp-buffer content
+     (goto-char (point-min))
+     (if (ruby-forward-string term nil t expand)
+         (should (= (point) index))
+       (should (null index)))))
+
+ (ert-deftest ruby-forward-string-accepts-paired-delimiters ()
+   (ruby-forward-string-should-move-to "<foo>bar" "><" 6)
+   (ruby-forward-string-should-move-to "[foo]bar" "][" 6)
+   (ruby-forward-string-should-move-to "(foo)bar" ")(" 6))
+
+ (ert-deftest ruby-forward-string-accepts-single-delimiters ()
+   (ruby-forward-string-should-move-to "/foo/bar" "/" 6)
+   (ruby-forward-string-should-move-to "|foo|bar" "|" 6)
+   (ruby-forward-string-should-move-to "-foo-bar" "-" 6))
+
+ (ert-deftest ruby-forward-string-accepts-carets ()
+   :expected-result :failed
+   (ruby-forward-string-should-move-to "^foo^bar" "^" 6))
+
+ (ert-deftest ruby-forward-string-scans-the-shortest-match ()
+   (ruby-forward-string-should-move-to "<foo>"   "><" 6)
+   (ruby-forward-string-should-move-to "<foo>>"  "><" 6)
+   (ruby-forward-string-should-move-to "<foo><>" "><" 6))
+
+ (ert-deftest ruby-forward-string-skips-escaped-delimiters ()
+   (ruby-forward-string-should-move-to "<foo\\>"   "><" nil)
+   (ruby-forward-string-should-move-to "<foo\\>>"  "><" 8)
+   (ruby-forward-string-should-move-to "/foo\\/"   "/"  nil)
+   (ruby-forward-string-should-move-to "/foo\\//"  "/"  8)
+   (ruby-forward-string-should-move-to "/foo\\\\/" "/"  8))
+
+ (ert-deftest ruby-forward-string-requires-matched-delimiters ()
+   (ruby-forward-string-should-move-to "<foo"    "><" nil)
+   (ruby-forward-string-should-move-to "<foo<"   "><" nil)
+   (ruby-forward-string-should-move-to "<foo<>"  "><" nil)
+   (ruby-forward-string-should-move-to "<foo<>>" "><" 8)
+   (ruby-forward-string-should-move-to "<<><>>"  "><" 7)
+   (ruby-forward-string-should-move-to "><"      "><" nil)
+   (ruby-forward-string-should-move-to "/foo"    "/"  nil))
+
+ (ert-deftest ruby-forward-string-can-skip-interpolations ()
+   (ruby-forward-string-should-move-to "<f#{>}o>" "><" 6 nil)
+   (ruby-forward-string-should-move-to "<f#{>}o>" "><" 9 t)
+   (ruby-forward-string-should-move-to "/f#{/}o/" "/"  6 nil)
+   (ruby-forward-string-should-move-to "/f#{/}o/" "/"  9 t))

  (provide 'ruby-mode-tests)

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

[Dmitry Gutov]

Hi Cameron,

Cameron Desautels <camdez@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?

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

[Cameron Desautels]

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@gmail.com>


On Fri, Dec 6, 2013 at 9:22 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> Hi Cameron,
>
> Cameron Desautels <camdez@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?

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

[Dmitry Gutov]

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.

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

[Cameron Desautels]

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

On Sun, Dec 8, 2013 at 10:07 PM, Dmitry Gutov <dgutov@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@gmail.com>

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

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

[Dmitry Gutov]

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.

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

[Dmitry Gutov]

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.