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

Code style #4

Closed
paulirish opened this issue Mar 11, 2016 · 9 comments · Fixed by #5
Closed

Code style #4

paulirish opened this issue Mar 11, 2016 · 9 comments · Fixed by #5

Comments

@paulirish
Copy link
Member

I think we want Blink style for the JS. What do others think?

Perhaps we can extend https://github.com/google/eslint-config-google

I'd be happy with a line length max > 110 though. :)

@paulirish
Copy link
Member Author

I'm a BIG proponent of an auto-formatter, too.

https://github.com/angular/clang-format is one direction we could go.. now that it does support JS.

@paullewis
Copy link
Contributor

I'm mostly okay with the Google style. My only pref is to stick to 80 chars. Longer than that tends to unreadable code.

@brendankenny
Copy link
Member

#5 adds base Google style eslint checking. A couple of issues to address. Biggest thing is standard style putting a space before the parameter list and some excess white space. I'm happy to go through the current errors and do a quick auto-fix for those.

There are a handful of things that may need some bikeshedding.

This was referenced Mar 11, 2016
@brendankenny
Copy link
Member

Remaining eslint warnings for current code:

  • Line length. Maybe 100 characters as a compromise? I personally like a line limit but I don't care very much how long it is.
  • TODOs and FIXMEs. At first I was just going to remove this rule, but it is kind of handy to have something to nag us if we're going to leave them in. Bugs planned to be long term should really be in issues, not in the code. On the other hand: annoying. Thoughts?
  • Missing JSDoc comments. Looks like maybe if you start JSDocing a file eslint wants you to finish, since this warning only shows up in one file. Or maybe it's a es5/es2015 thing? Obviously this is a larger conversation re: doc strings and building tools, so happy to suppress (or not) this warning for now.

@brendankenny
Copy link
Member

investigated clang-format today. For posterity's sake, you can recreate my results with something like npm install --save-dev clang-format (includes Win, Linux, and Mac binaries), creating a .clang-format file with something like:

---
Language: JavaScript
BasedOnStyle: Chromium
ColumnLimit: 100
AllowShortFunctionsOnASingleLine: Empty
TabWidth: 2
UseTab: Never

and then run with node_modules/.bin/clang-format -i --glob='{,./!(node_modules)/**/}*.js'

output is...OK. It does some screwy things with object and array literals and arrow functions, sometimes putting lots of things on one line and sometimes not. There may be an additional rule to fix those that I'm missing, but I couldn't find it. .thens it really likes to put on one line if not using an inline function, which is something I'm not wild about either.

It's nice to have a tool that fixes a bunch of style issues automatically, but not sure if it's worth it.

@paullewis
Copy link
Contributor

I'd prefer sticking with something like .eslintrc, just because that's what the majority of people use. Ideally people from the community can and should contribute as they wish, and the less ... unusual the better imo.

@paulirish
Copy link
Member Author

paulirish commented Mar 15, 2016 via email

@brendankenny
Copy link
Member

yeah, just to be clear, the hope was to be able to configure clang-format so that it would fix violations of the exact same rules as in the .eslintrc file, so it would be an option for those who e.g. need a robot to hit the semicolon key for them.

Looks like clang-format is too pushy on other fronts for that to work out, though.

@paulirish
Copy link
Member Author

  • Let's proceed with line limit of 80. :)
  • TODOS and FIXMEs will remain warnings. seems good.

groovy. i think we're set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants