bug#25904: Formatting bug with js-mode

[Dmitry Gutov <dgutov@yandex.ru>]

On 11/21/17 12:22 AM, Felipe Ochoa wrote:

> I've added a test to js.js in the latest patch below.

Thanks.

>> To allow comments between the opening paren and the arglist? Does 
>> anybody write them there?
> 
> Oops -- this comment was supposed to be after the arrow. I was thinking 
> of return type annotation comments, but I just checked flow and I don't 
> think they support this. We can just remove the comment

Sure.

> Is there an arglist regexp already in use somewhere? I'd rather not roll 
> my own since dealing with default argument values and spreads and such 
> seems quite challenging.

No, sorry, I forgot about destructuring and etc. forward-list is fine here.

>> If it's not one regexp, moving some of the new code into a helper 
>> function (with a sensible name) seems prudent.
> 
> I've done this in the latest patch.

Thanks.

> +(defun js--looking-at-broken-arrow-function-p ()
> +  "Helper function for `js--proper-indentation'.
> +Return t if point is at the start of a (possibly async) arrow
> +function and the last non-comment, non-whitespace token of the
> +current like is the \"=>\" token."
> +  (and (looking-at "\\s-*\\(async\\s-*\\)?")

This looks weird: the regexp will always match. Better to write it as

(if (looking-at NON-OPT-REGEXP) (goto-char ...)), where NON-OPT-REGEXP 
does not include the optional qualifier (?).

And the (save-excursion...) form seems unnecessary, since the caller 
already uses it.

> +       (save-excursion
> +         (goto-char (match-end 0))
> +         (cond
> +          ((eq (char-after) ?\()
> +           (forward-list) ; Is this better than forward-sexp?

I think so: it should be a bit faster, and it doesn't require you to 
bind forward-sexp-function to nil (for e.g. js2-mode).

> +           (looking-at-p "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
> +          (t (looking-at-p
> +              (concat js--name-re "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))

Should we extract "\\s-*=>\\s-*\\(/[/*]\\|$\\)" to a local variable, or 
a constant?

> (defun js--proper-indentation (parse-status)
>    "Return the proper indentation for the current line."
>    (save-excursion
> @@ -2107,7 +2124,12 @@ indentation is aligned to that column."
>                   (continued-expr-p (js--continued-expression-p)))
>               (goto-char (nth 1 parse-status)) ; go to the opening char
>               (if (or (not js-indent-align-list-continuation)
> -                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
> +                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
> +                     (when (eq (char-after) ?\()
> +                       (save-excursion
> +                         (forward-char)
> +                         (skip-syntax-forward " ")

This seems a bit extraneous: the regexps in the called function all 
start with "\\s-*", right? Let's maybe have the one or the other.

> +                         (js--looking-at-broken-arrow-function-p))))
>                   (progn ; nothing following the opening paren/bracket
>                     (skip-syntax-backward " ")
>                     (when (eq (char-before) ?\)) (backward-list))
> diff --git a/test/manual/indent/js.js b/test/manual/indent/js.js
> index b0d8bca..2286cc1 100644
> --- a/test/manual/indent/js.js
> +++ b/test/manual/indent/js.js
> @@ -144,6 +144,15 @@ bar(
>    /abc/
> )
> 
> +// #25904

We write references to debbugs as bug#25904. bug-reference-mode can 
linkify the result.

> +foo.bar.baz(very =>
> +  very
> +).biz(([baz={a: [123]}, boz]) =>
> +  baz
> +).snarf((snorf) =>
> +  snorf
> +);
> +

Thank you.

Please attach the resulting patch as a file produced by 'git 
format-patch', with a ChangeLog style message entry, as described in 
CONTRIBUTE.