Mr.doob's Code Style™ #4802

Open
zz85 opened this Issue May 12, 2014 · 30 comments

Projects

None yet

7 participants

@zz85
Contributor
zz85 commented May 12, 2014

I wrote a gist describing a little of mrdoob's code conventions (as some have referred to the MDCS) and included a jscs rules for code style enforcing. https://gist.github.com/zz85/e929503387cdc597b4f7

Not everyone is familiar or used to MDCS so @WestLangley asked if there's a script which can "mrdoobify" the rest of our codebase which has not conformed to that standard.

So some objectives I hope to archive are:

  1. Have some documentation on mrdoob's code style (perhaps on the wiki pages?)
  2. Add jscs rules so contributors can use it for validating code via pre-commit hook or editor's linting (sublime / vim) or just part of the build tools.
  3. Try some code transformation tools to make existing code more mrdoob style compliant.

For 2, there are a couple more features in the JSCS I'm waiting to get implemented or merged before it could really enforce MDCS. In particular, jscs-dev/node-jscs#371 jscs-dev/node-jscs#333 possibly jscs-dev/node-jscs#244 and jscs-dev/node-jscs#126 (comment) too.

As for 3, I could try out codepainter (https://github.com/jedmao/codepainter) and see how well it works for this.

@WestLangley
Collaborator

Currently, the only reference to the coding style is the request to

Format whitespace consistently with the rest of code base

in the Wiki article How to Contribute to three.js

A tool that "mrdoobified" the entire codebase would aleviate the requirement for this request entirely.

@mrdoob
Owner
mrdoob commented May 12, 2014

Actually, if there was a plugin of sorts I could add to sublime that would automatically format the code that would be the end of the problem :)

@mrdoob mrdoob added the Enhancement label May 12, 2014
@zz85
Contributor
zz85 commented May 13, 2014

actually both node-jscs and codepainter are built on top of esprima (https://github.com/ariya/esprima) which is very powerful because its builds an AST (abstract syntax tree) of the code, at the same time probably more difficult to deal with.

one easy alternative is to try a list of regex or common search and replace rules eg.
if (bla) -> if ( bla )

@zz85
Contributor
zz85 commented May 13, 2014

I finally got codepainter working, its cli commands are pretty sensitive so it wouldn't work if some flags are wrong.

first create a json eg. mrdoob_codepainter.json

{
    "indent_style": "tab",
    "indent_size": "tab",
    "insert_final_newline": true,
    "quote_type": "auto",
    "space_after_anonymous_functions": true,
    "space_after_control_statements": true,
    "spaces_around_operators": true,
    "trim_trailing_whitespace": true,
    "spaces_in_brackets": "hybrid",
    "end_of_line": "lf"
}

then

codepainter xform -j ../utils/codestyle/mrdoob_codepainter.json  "*/*.js"

The nice thing is that it transform a whole bunch of js files. The bad thing is that it doesn't really conforms to MDCS. In particularly, the "hybrid" mode for spaces_in_brackets follows the idiomatic js style.

@mikesherov

@zz85 , JSCS may have an autoformatting mode soon. Stay tuned.

@zz85
Contributor
zz85 commented May 17, 2014

that's awesome @mikesherov! any timeline on that? :D

autoformatting probably just one of the few things I missed using with java in eclipse.

@gero3
Contributor
gero3 commented Jun 24, 2014

@mrdoob If a plugin existed for sublime text 2, would that be enough??

@mrdoob
Owner
mrdoob commented Jun 24, 2014

I would definitely be enough for me! ^^

@gero3
Contributor
gero3 commented Jun 24, 2014

Do you mind the need of node.js?? Otherwise it is a bit harder to do.

@mrdoob
Owner
mrdoob commented Jun 24, 2014

Requiring node.js is ok :)

yum install nodejs
@gero3
Contributor
gero3 commented Jun 24, 2014

first version:
https://github.com/gero3/Mrdoob-s-code-style-sublime-plugin

It works but only on the three.js repository and no checks at all. It only works at the src folder and no single file.

@zz85
Contributor
zz85 commented Jun 30, 2014

nice start @gero3.. i've previously wrote a plugin for sublime3 which basically subclass of sublimeLinter for just doing validations (jshints/jscs). i'm not too sure what's the best approach for code formatting for now (eg. wait for jscs to have autoformat which i assume will take a while, use code painter which isn't that well supported, or handroll as solution with esprima and escodegen for this).

@gero3
Contributor
gero3 commented Jul 2, 2014

I'm starting to think a combination of options 1 and 3 would be for the best. Use jscs with some additional rules for MDCS support( It doesn't look that hard) and write our own formatter. I can't even get codepainter to work within a node envirment.

@mikesherov

By the way, we'd gladly accept a PR to JSCS that implements Mr. Doob's Code Style, even including any additional rules needed to enforce it.

I'd help in any way you need to see it through.

@gero3
Contributor
gero3 commented Jul 2, 2014

@mikesherov The biggest problem I see is that errors don't have any indication to indicate from which rule they are from. I would suggest keeping the getoptionname inside the errors too.

@mikesherov

@gero3 that's planned for the upcoming release!

@gero3
Contributor
gero3 commented Jul 2, 2014

We don't mind using a dev version. So if you need any help with the next release, I would love to contribute to it. Is contributing to mdevils/node-jscs on the master branch enough??

@mikesherov

@gero3, yes. The best way to contribute is submit a pull request there that brings with it a preset for MDCS using the existing (although incomplete) rules that apply.

From there we can see about about adding new rules for further coverage.

@zz85
Contributor
zz85 commented Jul 2, 2014

+1 for mdcs preset in jcsc :)

@gero3 i think jscs have update several of its rules since the last time i used it, we probably should update https://gist.github.com/zz85/e929503387cdc597b4f7 for it updated rules

@mikesherov i still find writing jscs rules a little tricky.. sometimes i wonder if i need to write another set of test cases (apart from jscs unit tests) to make sure that my presets are validating my styled source files correctly...

@gero3
Contributor
gero3 commented Jul 3, 2014

I made MDCS work again in jscs. I also added a preset: jscs-dev/node-jscs#496.
Also now we can format the source code again when jscs-dev/node-jscs#492 goes through

@mrdoob
Owner
mrdoob commented Jul 3, 2014

I had a quick try at atom yesterday. Seems like it's getting pretty decent, and I suspect it may be easier to make this work with it?

@zz85
Contributor
zz85 commented Jul 4, 2014

cool @gero3, looks like work on MDCS brings a couple of improvements to jscs.

@mrdoob i tried atom a little, but i haven't found enough for me to switch over from sublime (the same for brackets). that said, the idea of everything in js/coffeescript for an editor is interesting, and there are atom plugins already for linting like linter-jscs and jshint

@gero3
Contributor
gero3 commented Jul 6, 2014

I documented the Mr.doob's CodeStyle™ on the wiki

https://github.com/mrdoob/three.js/wiki/Mr.doob's-Code-Style%E2%84%A2

@mikesherov

FYI, MDCS is now in the released version of JSCS: https://github.com/mdevils/node-jscs/releases/tag/v1.5.9

@makc
Contributor
makc commented Jul 30, 2014

just a random "let us reinvent the wheel" idea: why not just do escodegen fork that would then format your js exactly the way you want it? I think it works like node module too, so you could indeed add this to atom easily.

@zz85
Contributor
zz85 commented Aug 6, 2014

👍 Great stuff @gero3 @mikesherov :)

@makc you still need to mangle the AST before running it thru escodegen. that is a little like what esmangle do.

@zz85
Contributor
zz85 commented Dec 28, 2014

Created a simple online tool to check for mrdoob style using browserified jscs

http://zz85.github.io/mrdoobapproves/

@mrdoob
Owner
mrdoob commented Nov 17, 2015

I've just started using https://atom.io/packages/linter-jscs with the MDCS preset 😮 Thanks guys!

Out of curiosity... Is there any package that shows unused variables?

@zz85
Contributor
zz85 commented Nov 17, 2015

I've just started using https://atom.io/packages/linter-jscs with the MDCS preset 😮 Thanks guys!
Nice, I guess it should have been time there's is a decent plugin integration for a popular editor ;)

Out of curiosity... Is there any package that shows unused variables?

I don't remember JSCS doing that, the usual linters jslint/jshint/eslint etc does that. IIRC, the current project which allows you to simply check unused without enforcing all other rules would be http://eslint.org/

It also turns out that an an unused variable checker is something not too difficult to write. I wrote one for JS (using the esprima tokens) and then another for glsl).

@zz85 zz85 referenced this issue in zz85/mrdoobapproves Nov 25, 2015
Closed

Lint unused variables #48

@abelnation
Contributor

see: #8805

also, eslint is a great modern tool for js linting. jscs seems to actually have merged w/ the eslint group, so not sure what the future of that tool is. would be nice to be able to specify both linting rules and get a resulting auto-formatter for free

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment