Re: [PATCH] Update js-mode function heading regexes

[Dmitry Gutov]

Hi Adam,

Sorry for the late reply.

On 6/19/17 10:04 AM, Adam Niederer wrote:

> The file I included was primarily for visual evaluation of the changes.
> Adding some automated tests is a good idea, though, so I've included an
> updated patchset below. This also makes js--function-heading-2-re work
> on async functions.

Sometimes it's easier to evaluate the improvement just by looking at the 
tests, though it depends on how they are written, of course.

To comment on the patch and your original post:

- The change to heading-3-re is not mentioned, but it's fairly obvious.

- I don't quite understand the description of the change in 
heading-2-re. Why not at least require a colon?

> > Not much. Patch submissions via the bug tracker have higher odds of
> > not being forgotten is they're not applied right away, though.
> >
> > Thanks for the patch.
>
> Would reposting these changes to bug-gnu-emacs be a good idea?

By now it's looking even better than it did back then. As you can see, 
it's been lost in the mailing list.

And I'd like for someone else to look at the patch, because it touches 
the area of js-mode than I've never had to deal with.

> +(ert-deftest js-mode-highlight-function-names ()
> +  "Ensure js--function-heading-1-re and js--function-heading-2-re match all
> +possible function declarations and highlight the function names
> accordingly."

LGTM.

> +(ert-deftest js-mode-accept-function-bindings ()
> +  "Ensure js--function-heading-3-re matches all function declarations
> of the
> +form (var|let|const) foo = (async)? function."

Regarding this test, can we check something higher level than whether 
js--function-heading-3-re matches? It's an implementation detail, after all.