* bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH)
@ 2013-12-06 17:15 Cameron Desautels
2013-12-07 3:22 ` Dmitry Gutov
0 siblings, 1 reply; 7+ messages in thread
From: Cameron Desautels @ 2013-12-06 17:15 UTC (permalink / raw)
To: 16078
[-- 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)
^ permalink raw reply [flat|nested] 7+ messages in thread* bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH)
2013-12-06 17:15 bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH) Cameron Desautels
@ 2013-12-07 3:22 ` Dmitry Gutov
2013-12-08 2:29 ` Cameron Desautels
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Gutov @ 2013-12-07 3:22 UTC (permalink / raw)
To: Cameron Desautels; +Cc: 16078
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH)
2013-12-07 3:22 ` Dmitry Gutov
@ 2013-12-08 2:29 ` Cameron Desautels
2013-12-09 4:07 ` Dmitry Gutov
0 siblings, 1 reply; 7+ messages in thread
From: Cameron Desautels @ 2013-12-08 2:29 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 16078
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?
^ permalink raw reply [flat|nested] 7+ messages in thread* bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH)
2013-12-08 2:29 ` Cameron Desautels
@ 2013-12-09 4:07 ` Dmitry Gutov
2013-12-11 22:48 ` Cameron Desautels
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Gutov @ 2013-12-09 4:07 UTC (permalink / raw)
To: Cameron Desautels; +Cc: 16078
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH)
2013-12-09 4:07 ` Dmitry Gutov
@ 2013-12-11 22:48 ` Cameron Desautels
2013-12-15 3:36 ` Dmitry Gutov
0 siblings, 1 reply; 7+ messages in thread
From: Cameron Desautels @ 2013-12-11 22:48 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 16078
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH)
2013-12-11 22:48 ` Cameron Desautels
@ 2013-12-15 3:36 ` Dmitry Gutov
2015-11-15 14:55 ` Dmitry Gutov
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Gutov @ 2013-12-15 3:36 UTC (permalink / raw)
To: Cameron Desautels; +Cc: 16078
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-15 14:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-06 17:15 bug#16078: Extensive docs and tests for `ruby-forward-string' (PATCH) Cameron Desautels
2013-12-07 3:22 ` Dmitry Gutov
2013-12-08 2:29 ` Cameron Desautels
2013-12-09 4:07 ` Dmitry Gutov
2013-12-11 22:48 ` Cameron Desautels
2013-12-15 3:36 ` Dmitry Gutov
2015-11-15 14:55 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).