Skip to content
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

Open
carlmjohnson opened this Issue Aug 31, 2017 · 24 comments

Comments

Projects
None yet
9 participants
@carlmjohnson
Copy link
Contributor

carlmjohnson commented Aug 31, 2017

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.

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

carlmjohnson commented Aug 31, 2017

@andybons expressed interest in this while reviewing a CL for #21643.

@ianlancetaylor ianlancetaylor changed the title Proposal x/tools: Use a formatter on JS included in project proposal: x/tools: use a formatter on JS included in project Aug 31, 2017

@gopherbot gopherbot added this to the Proposal milestone Aug 31, 2017

@gopherbot gopherbot added the Proposal label Aug 31, 2017

@OneOfOne

This comment has been minimized.

Copy link
Contributor

OneOfOne commented Sep 1, 2017

Please use tabs instead of spaces to match gofmt, please, please, please.

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Sep 1, 2017

Oh no what have we done

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Oct 16, 2017

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.

@rsc rsc changed the title proposal: x/tools: use a formatter on JS included in project x/tools: agree to use a formatter on JS included in project Oct 16, 2017

@rsc rsc changed the title x/tools: agree to use a formatter on JS included in project x/tools: agree to use a specific JS formatter Oct 16, 2017

@andybons andybons self-assigned this Oct 16, 2017

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 18, 2018

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, package.json, node_modules need to be added to .gitignore and all the baggage that comes with it. With just a handful no. of js files in our repos, I don't see a need to check these in the repo.

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.

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

carlmjohnson commented Apr 18, 2018

Or someone could write a JS code formatter in Go. :-)

@opennota

This comment has been minimized.

Copy link

opennota commented Apr 19, 2018

standard.js removes semicolons by default (which is probably not what we want); and eslint:recommended wouldn't complain about lack of spaces around operators: var a =1+ 2;

And the up-and-coming prettier formatter is more or less like gofmt.

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

carlmjohnson commented Apr 19, 2018

prettier is to gofmt as eslint-recommended is to go vet.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 25, 2018

Looks like I have fallen behind on the latest JS tool-of-the-week. Took prettier out for a spin. I am good with it. Only minor change that is needed is that we need to pass a flag to use tabs, because it uses spaces as default. A combination of prettier and eslint:recommended for formatting and vetting sounds good to me.

@OneOfOne

This comment has been minimized.

Copy link
Contributor

OneOfOne commented Apr 25, 2018

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.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 25, 2018

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.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 28, 2018

Ok, spent some time and evaluated these tools in some more detail.

I have kept the following rules in mind during the evaluation process:

  • Should need bare minimal config knobs.
  • Attempt to conform to Go code style.
  • Overall, it should follow the Go philosophy of simplicity and minimalism.

Tools evaluated:

  • Standard.js - Formats only js files. Uses a fixed set of eslint rules under the hood.
  • Eslint:recommended - Main job is to function as a linter. The "recommended" rule set contains a default set of rules to be checked against.
  • Prettier - Used purely as a formatter. Operates on a lot of languages including js/css. Doesn't do linting.

Points to be noted:

  • Both standard.js and prettier use spaces by default. Standard.js doesn't expose a way to customize that (you can customize the eslint rule underneath, but that doesn't change the formatter; at least from what I have seen).

  • Standard.js removes semicolons, prettier doesn't by default (can be customized).

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 - --use-tabs and --no-semi.

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. eslint:recommended fits that role nicely. But the primary focus in these repos is to write Go code. There are just a handful of js files, and proposing guidelines which cannot be automatically fixed by developers is frankly a waste of time for so little js code. I have actually run eslint:recommended on the codebase, and it mostly gives errors for undefined global variables. A minor thing to consider given the fact that it's majorly a Go codebase.

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 eslint:recommended.

PS: Although you can use prettier along with eslint to do both formatting and linting, you need to take care of conflicting rule sets. And then further packages need to be installed to take care of that (https://prettier.io/docs/en/eslint.html). I feel this only complicates things and prevents a clean separation of concerns between code formatting and vetting.

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.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

jimmyfrasche commented Apr 28, 2018

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.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 28, 2018

Is removing semicolons just to make the js more like Go code?

Yes. IMO, the code looks much cleaner. Although this is pure bikeshedding territory. We should just choose one and go with it.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

jimmyfrasche commented Apr 28, 2018

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.

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Apr 28, 2018

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.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 29, 2018

JS is not Go. We shouldn’t make decisions in the interest of making the former look like the latter.

Cool, no worries.

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.

I would love that too. Unfortunately, I don't see any js/css formatter written in Go.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

jimmyfrasche commented Apr 29, 2018

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.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 29, 2018

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.

Yep, right. It would be mentioned in the README what version to install. Since we wouldn't be checking in package.json.

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

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.

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Apr 30, 2018

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/

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 30, 2018

It also is just churn and muddies up git blame.

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.

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Apr 30, 2018

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."

@kelwang

This comment has been minimized.

Copy link

kelwang commented May 4, 2018

also need to bypass minified js

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented May 12, 2018

Docker image agniva/prettify at your service.

docker pull agniva/prettify

13:21:41-agniva-~/play/go/src/golang.org/x/tools$docker run --rm -it --volume "$PWD":/wd  agniva/prettify "**/*.{js,css}"
cmd/present/static/article.css 155ms
cmd/present/static/dir.css 35ms
cmd/present/static/dir.js 35ms
cmd/present/static/notes.css 9ms
cmd/present/static/notes.js 40ms
cmd/present/static/slides.js 137ms
cmd/present/static/styles.css 104ms
godoc/static/godocs.js 119ms
godoc/static/play.js 28ms
godoc/static/playground.js 88ms
godoc/static/style.css 106ms

Once you alias it to the prettier command - alias prettier='docker run --rm -it --volume "$PWD":/wd agniva/prettify'. Then it becomes -

$prettier "**/*.{js,css}"

Source - https://gist.github.com/agnivade/5bc1987c4ef954eb63627679849c9da5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.