New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/tools: agree to use a specific JS formatter #21719
Comments
Please use tabs instead of spaces to match gofmt, please, please, please. |
Oh no what have we done |
Has the JavaScript community agreed on a standard formatter? I don't see a reason for us to check one into our repo. It would be fine - if everyone agrees - to have a soft standard that maybe the JS should be formatted by , best effort. There are only 21 JS files in golang.org/x. Leaving for @andybons to approve a specific formatter (or lead a discussion toward approving one). If the proposal is just 'agree that we should format JS using tool X' and not 'force this in some kind of presubmit or automated tooling', then proposal accepted, pending @andybons and others picking a formatter. |
Coming here from #24933. The 2 most popular tools in js linting and formatting space is standard js and eslint:recommended. We can decide on any one of them and just go with it. But my biggest complaint is that using these tools will involve the entire npm ecosystem. You will need a nodejs installation, Maybe we can come to a middle ground and document what formatter we use. The developer just runs the tool manually before pushing a CL. And once before every release, somebody can run a manual check to confirm if everything is according to standard, and if not, push a separate check-in. Or something like that. |
Or someone could write a JS code formatter in Go. :-) |
And the up-and-coming |
prettier is to gofmt as eslint-recommended is to go vet. |
Looks like I have fallen behind on the latest JS tool-of-the-week. Took |
Prettier's extremely opinionated defaults that you can't change are annoying, I ended up just using eslint + airbnb's config + sane custom settings that resemble gofmt. My config is more es6/ts though, but I'd gladly help and work with anyone if they wanna adapt it / change it. https://gist.github.com/OneOfOne/c62e964cc159c22f36ce5dd535de2270#file-eslintrc-js @carlmjohnson while that's kinda true, you can 100% just use eslint for formatting without prettier. |
I ran it on a couple of js and css files. Nothing felt too annoying to me. Everybody has their own opinions on what is annoying and not. Even I recommended initially to go with eslint / standard.js. Whatever we choose, we wouldn't want to have configuration to customize the rules. I think the point is to just choose one and go with the defaults. Maybe one or two customizations at most (like tabs instead of spaces); not more than that. |
Ok, spent some time and evaluated these tools in some more detail. I have kept the following rules in mind during the evaluation process:
Tools evaluated:
Points to be noted:
Given all the above, prettier is the closest alternative to gofmt. And to remain as close as possible to the Go-style of code, it just needs 2 knobs - I propose we go ahead with prettier as the formatter. Regarding linting, we just need something to print out the failures against a fixed rule set. I would recommend not to use any linting at all, or at most mention a one-liner that it is an added bonus if your js code passes PS: Although you can use I have already run prettier on the godoc repo and the resulting output looks pretty clean and neat. The resultant code size increases slightly because of the added newlines and tabs. But once we start using a minifier (which I have plans for !), this should be taken care of. Over to @andybons for a final decision. |
Is removing semicolons just to make the js more like Go code? I don't see the point and semicolon insertion in js is fraught. I can see sticking to tabs consistently to avoid having to toggle soft tabs on and off in editors when moving between languages in the same codebase and it being hard to notice if you did not, though. |
Yes. IMO, the code looks much cleaner. Although this is pure bikeshedding territory. We should just choose one and go with it. |
If the goal is to avoid bikeshedding, then we should just accept all of the tool's default settings unless there's a pragmatic argument for requiring a specific setting. |
Sorry, but we’re not removing semi-colons from JS. JS is not Go. We shouldn’t make decisions in the interest of making the former look like the latter. Rather than spending time debating the merits of specific knobs to turn, I’d rather have solutions for when a dev doesn’t want to install npm and a gillion dependencies on their machine in order to format their code. Let’s please focus on that particular problem. |
Cool, no worries.
I would love that too. Unfortunately, I don't see any js/css formatter written in Go. |
There's also version skew in the formatter to consider. If I'm using version X and you're using version Y, they may format the same code differently. A complete solution would be to have a bot that formats CLs which would always be a single fixed version and configuration (and set to ignore vendored resources like jquery). It would probably suffice—at least for now—for someone to run the formatter on all relevant sources once and then afterwards we just endeavor to make new code look like old code and call out dissimilar formatting in reviews. |
Yep, right. It would be mentioned in the README what version to install. Since we wouldn't be checking in package.json.
I was suggesting to go one step further and just run the formatter from time to time to keep the codebase clean. Code reviews are difficult as it is, we wouldn't want to add additional load of reviewing style too. Regarding the issue of installing the node dependencies, my thinking was that since this is not a hard requirement in the first place, nobody is required to install node/npm at all. Just the install steps will be mentioned in the README (Assuming we will be using the node ecosystem in the first place). If someone is willing to do it, great, otherwise no hard feelings. I am willing to take point in running the formatter probably once a month to check and see if anything has fallen out of line. |
Adding an additional step that must be performed by a single person every month seems like a step backwards from adding a formatter in the first place. It also is just churn and muddies up git blame. Many of the devs that do development on these products may have docker installed. Could we dockerize the command so that the user only needs to download an image and run that? See https://spin.atomicobject.com/2015/11/30/command-line-tools-docker/ |
Hmm .. how does actually installing node/npm, vs running it through docker make a difference to the git blame ? The code change will still be the same right ? Personally, I feel installing docker to be an extra mental overhead. It reduces my ability to debug things. But I can see how it can be easy for some users. If you agree that we should use prettier, I can probably investigate and see if it works out. |
Dockerizing the command and the git blame issue are separate concerns. Having someone go through the codebase every month reformatting things doesn't scale, has a bus factor of 1, and will muddy up git blame. The docker issue is separate from this. I should have said "provide an option to download a dockerized version of the command so that those who don't want to download npm, node, and the world will have an easy way of avoiding those steps." |
also need to bypass minified js |
Docker image agniva/prettify at your service.
Once you alias it to the
Source - https://gist.github.com/agnivade/5bc1987c4ef954eb63627679849c9da5 |
@andybons - Now that work is going on with the website, any thoughts on this ? I guess you are already manually using |
If I were starting a new project, the following would be in the
|
Sure, that can be done. I was asking more on how can we actually move ahead and formalize this. Should I push the |
Let’s start by documenting it and if someone wants to do a pass on all the JS that’s fine. I’m less worried about git blame now as long as it’s not part of an unrelated change. |
Change https://golang.org/cl/183877 mentions this issue: |
Change https://golang.org/cl/183878 mentions this issue: |
Change https://golang.org/cl/183879 mentions this issue: |
Change https://golang.org/cl/183880 mentions this issue: |
I apologize for the delay on reviewing the CLs here. I’ve been thinking about this a lot. In practice, people simply don’t read (despite our best efforts). We should keep that in mind when we introduce this requirement. My first inclination is to check in the prettier config and encourage the small group of people making changes to the JS/CSS to configure their editor accordingly. Is there a way to specify that we only want to format .js and .css files for now? Go templates get messed up when prettier thinks they’re html (and I’d rather not change our suffixes in some cases) |
|
Yep. See my CLs which change the READMEs. Just |
Updates golang/go#21719 Change-Id: I3709577f48bcbff26beb6f5b8ff8d06fb18cc619 Reviewed-on: https://go-review.googlesource.com/c/tools/+/183877 Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>
Command used: docker run --rm -it --volume "$PWD":/wd agniva/prettier "**/*.{js,css}" Updates golang/go#21719 Change-Id: I892718781206430a8f85da38a762b54191ce023a Reviewed-on: https://go-review.googlesource.com/c/tools/+/183878 Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>
Updates golang/go#21719 Change-Id: Ie33b439c929c466089398c4f24dc243239c48f10 Reviewed-on: https://go-review.googlesource.com/c/website/+/183879 Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>
Command used: docker run --rm -it --volume "$PWD":/wd agniva/prettier "**/*.{js,css}" Updates golang/go#21719 Change-Id: Ibd53141bb0032078953af873dde0b0c47e290fb4 Reviewed-on: https://go-review.googlesource.com/c/website/+/183880 Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>
The READMEs of both the website and tools repos are updated now, along with a checked-in With that, I am calling it a close. Thanks everyone for the discussion. |
I recently worked on #21643, which involved changing the JavaScript in the x/tools repo. This JavaScript has no clear guidelines for style and is written in an inconsistent way from file to file. Some of the semi-colons, e.g., in slides.js are redundant, and semi-colons are missing in other places.
I propose that we add a JavaScripter formatter and linter to the repo to remove these inconsistencies and prevent their recurrence. Prettier could be used as a JavaScript equivalent of gofmt, and ESLint could be used to prevent issues like unused variables, unintended global variables, etc.
The text was updated successfully, but these errors were encountered: