Auto-formatting JS code

[Andrew Grieve]

Just discovered that "git cl format --js" will format javascript with clang-format.

Seemed to work well on my one sample file.

Does anyone know why this is opt-in rather than on-by-default? Can we remove the need for the --js flag?

[dan...@chromium.org]

Blame says https://source.chromium.org/chromium/_/chromium/chromium/tools/depot_tools.git/+/c5ba692d1e7314884b9c17daf8922babffe44bf8 did it.

For bug https://bugs.chromium.org/p/chromium/issues/detail?id=567770

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiQX1VQ9vySMu3_Vng-eGs3jHpjOtzQFc3K7iff1kfcK%2By%2BPQ%40mail.gmail.com.

[Dirk Pranke]

Yeah, the problem here is that it relies on clang-format being present in the "normal" location for this to work. But, `git-cl format` by itself works for non-chromium-src repos as well, and might not have that binary.

A better fix would be to bundle clang-format separately and pull that in via CIPD into depot_tools, but I'm not filing a bug for that request at the moment.

-- Dirk

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaTFDad-nrhf7eYmJaos4v0Kc2ZeKnxt11Nv%2BnLEHtpzMQ%40mail.gmail.com.

[Nico Weber]

Why does running clang-format on js files have stronger requirements than running it on cc files? Shouldn't that work equally well?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEoffTCnBEAOJdESCYFze89W%2BbrrOr7xT7buQjJyuD39oig%2ByQ%40mail.gmail.com.

[Dirk Pranke]

What do you mean? I don't understand your question.

-- Dirk

[Nico Weber]

You said: 'Yeah, the problem here is that it relies on clang-format being present in the "normal" location for this to work. But, `git-cl format` by itself works for non-chromium-src repos as well, and might not have that binary.'

'it' probably means 'running clang-format on js files'.

`git cl format` is able to run clang-format on cc files. Behind the scenes, I think this finds clang-format in the chromium tree. So I think this only works for chromium (unless you set CHROMIUM_BUILDTOOLS_PATH).

So shouldn't it be possible to make that Just Work for .js files too? Or does `git cl format --js` do more than just run clang-format?

[Dirk Pranke]

On Thu, Feb 3, 2022 at 4:08 PM Nico Weber <tha...@chromium.org> wrote:

You said: 'Yeah, the problem here is that it relies on clang-format being present in the "normal" location for this to work. But, `git-cl format` by itself works for non-chromium-src repos as well, and might not have that binary.'

'it' probably means 'running clang-format on js files'.

`git cl format` is able to run clang-format on cc files. Behind the scenes, I think this finds clang-format in the chromium tree. So I think this only works for chromium (unless you set CHROMIUM_BUILDTOOLS_PATH).

Right.

 

So shouldn't it be possible to make that Just Work for .js files too? Or does `git cl format --js` do more than just run clang-format?

I think it depends on what you mean by "just work". We could probably make it format JS without needing any command line switches in Chromium, but it doesn't do so currently.

Obviously, we could define some mechanism that would allow things to work correctly for arbitrary repos, but we haven't yet.

Does that get closer to what you were talking about?

[Nico Weber]

On Thu, Feb 3, 2022 at 7:41 PM Dirk Pranke <dpr...@google.com> wrote:

On Thu, Feb 3, 2022 at 4:08 PM Nico Weber <tha...@chromium.org> wrote:

You said: 'Yeah, the problem here is that it relies on clang-format being present in the "normal" location for this to work. But, `git-cl format` by itself works for non-chromium-src repos as well, and might not have that binary.'

'it' probably means 'running clang-format on js files'.

`git cl format` is able to run clang-format on cc files. Behind the scenes, I think this finds clang-format in the chromium tree. So I think this only works for chromium (unless you set CHROMIUM_BUILDTOOLS_PATH).

Right.

 

So shouldn't it be possible to make that Just Work for .js files too? Or does `git cl format --js` do more than just run clang-format?

I think it depends on what you mean by "just work". We could probably make it format JS without needing any command line switches in Chromium, but it doesn't do so currently.

Obviously, we could define some mechanism that would allow things to work correctly for arbitrary repos, but we haven't yet.

Does that get closer to what you were talking about?

I'm wondering why we have an opt-in switch for js but not for c++. Is that for historical reasons?

[Takuto Ikuta]

I guess that is mainly for infra repositories having javascript code but not having clang-format.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiF91JU2__bE5Ke601ZPU3zY9%2B20R0wmuZWoEd8H3ShuEQ%40mail.gmail.com.

[Takuto Ikuta]

I think so. I'm not sure how many js files we have, but there are infra repositories having some js, but no c++.

On Fri, Feb 4, 2022 at 12:06 PM Nico Weber <tha...@chromium.org> wrote:

So the argument is that non-chromium repos have quite a bit of js, but almost no c++?

[Nico Weber]

So the argument is that non-chromium repos have quite a bit of js, but almost no c++?

On Thu, Feb 3, 2022 at 10:02 PM Takuto Ikuta <tik...@chromium.org> wrote:

[Dirk Pranke]

Yes, I think this is right as well.

-- Dirk

[Nico Weber]

Maybe it could be an option to format js by default if clang-format can be found in the right place, or if there's a per-repo file to opt in?

(Assuming clang-format works well for js -- no idea!) 

[K. Moon]

The only thing "git cl format --js" does is add ".js" and ".ts" to the list of files clang-format will process:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/depot_tools/git_cl.py;l=5213;drc=c5f0cbb8654b3c38dea5714e117c71bd7ab6136f

The only advantages I can see to leaving out --js right now:

1. Save some CPU cycles by not formatting JavaScript/TypeScript files.

2. Don't mess with JavaScript/TypeScript files that aren't clang-formatted already.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiEPXtOC5ML2mdq_ZkFg5J_rL%3DHaJC0FPik074Qi5j97Cw%40mail.gmail.com.

[K. Moon]

While we're on the topic, it looks like clang-format acquired support to format JSON in the last year, which I'd also like to have. :-)

[Joe Mason]

On Fri, Feb 4, 2022 at 11:45 AM K. Moon <km...@chromium.org> wrote:

The only thing "git cl format --js" does is add ".js" and ".ts" to the list of files clang-format will process:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/depot_tools/git_cl.py;l=5213;drc=c5f0cbb8654b3c38dea5714e117c71bd7ab6136f

In that case, how about adding a per-repo config file with a value for CLANG_EXTS? (And possibly other options.) Then chromium could check in a file that includes .js and .ts, and other repos would get the hardcoded default. (And there could be a new "git cl format" option to ignore the args file and use the default.

Or, to follow the existing style, make --js work like --python (and add --no-js to mirror --no-python): if neither appear, include .js and .ts as long as a ".format_js" file appears in an ancestor directory. (In this case it can have arbitrary contents.)

The only advantages I can see to leaving out --js right now:

1. Save some CPU cycles by not formatting JavaScript/TypeScript files.

2. Don't mess with JavaScript/TypeScript files that aren't clang-formatted already.

Does the JS formatting work like the C++ formatting, only formatting lines that were touched by the current CL? (Unless --full is used)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACwGi-5n4_fLRCWW4GhNfxraLDyTjMC9kv6NmDXNHH0dxSHNpQ%40mail.gmail.com.

[K. Moon]

I have no input on the other points, but at the driver level, it looks like the code to invoke clang-format for JavaScript is the same as the code to invoke clang-format for C++, and already handles only formatting modified lines. (I think this is also my historical experience with git cl format --js.) I'd assume everything behaves the same, unless there's language-specific formatting in clang-format itself.

[Andrew Grieve]

Thanks for everyone's input. It doesn't sound like anyone has objections to turning this on in some fashion.

If the main concern is that some repo's don't have clang-format available, then I think one reasonable way might be to enable it when a ".clang-format" file exists.

I'd also propose changing the CheckPatchFormatted() canned check to work the same way (enable .js checking if .clang-format exists).

We've undergone a similar transition of no format enforcing -> format enforcing for python with YAPF. AFAICT, it's been a great change. Since the formatter and presubmit check complains only about changed lines, it removes a good amount of needless back & forth in code reviews about inconsistent whitespace.

I'll give this thread another day or so and then create a bug to track this.