Code Style Improvements #4806

zz85 · 2014-05-13T02:06:55Z

There's probably certain things which fell through the cracks, and several areas which I'm also not certain of @mrdoob's preferences. Nevertheless, first round attempt to code transform codebase mrdoob style using codepainter with some sed commands.

git reset --hard
codepainter xform -j ../utils/codestyle/mrdoob_codepainter.json  "*/*.js"
find ./ -type f -exec sed -i -e 's/( )/()/g;s/\[ \]/[]/g;s#{[ ]}#\{\}#g' {} \;
find . -name "*.js-e*" -print0 | xargs -0 rm
git add -p

See #4802
cc. @WestLangley

git reset --hard codepainter xform -j ../utils/codestyle/mrdoob_codepainter.json "*/*.js" find ./ -type f -exec sed -i -e 's/( )/()/g;s/\[ \]/[]/g;s#{[ ]}#\{\}#g' {} \; find . -name "*.js-e*" -print0 | xargs -0 rm git add -p

WestLangley · 2014-05-13T02:59:31Z

This is sweet!

Here are some things I noticed. Of course, it is @mrdoob's choice.

= - 1      not    = -1
- x + y    not    -x + y
c = - ( far + near )    not    c = -( far + near )
i ++       not    i++

Replacing " " with ' is fine, but you have to be careful in cases where there are nested quotes. Maybe you are...

*.html example files could use this treatment, too.

Why are the *.Loader files completely replaced?

mrdoob · 2014-05-15T00:20:57Z

@zz85 any chance you can implement @WestLangley suggestions and run the script again? O:)

zz85 · 2014-05-17T14:41:18Z

sorry for the delay.. haven't had much free time the last week. yeah, I could probably regex search the replace the spaces for negation and increments. @mrdoob, what's your take on the use of quotes?

JSCS rules: { "requireSpaceAfterPrefixUnaryOperators": [ "+", "-", "++", "--" ] }

JSCS rules: { "requireSpaceBeforePostfixUnaryOperators": ["++", "--"] }

zz85 · 2014-05-19T16:23:09Z

Ok made changes @WestLangley pointed.

Since auto-formatting is yet to be implemented (as @mikesherov has mentioned), I turned a custom error reporter in jscs into a code corrector for the unary operators.

jscs rule

{
    "requireSpaceAfterPrefixUnaryOperators": [ "+", "-", "++", "--" ],
    "requireSpaceBeforePostfixUnaryOperators": ["++", "--"]
}

space.js

var util = require('util');
var fs = require('fs');

/**
 * @param {Errors[]} errorsCollection
 */
module.exports = function(errorsCollection) {
    var errorCount = 0;
    /**
     * Fixes spacing errors.
     */

    var json = {};

    errorsCollection.forEach(function(errors) {
        var file = errors.getFilename();
        var file_contents = fs.readFileSync(file, 'utf-8');
        var lines = file_contents.split('\n');

        if (!errors.isEmpty()) {
           var list = [];

            errors.getErrorList().forEach(function(error) {
                list.push({
                    line: error.line,
                    column: error.column,
                    message: error.message
                })
                errorCount++;
            });

            json[file] = list;

            list = list.reverse();
            list.forEach(function(error) {
                var line = error.line - 1;
                var str = lines[line];
                var col = error.column;
                while (str.charAt(col) != '+' && str.charAt(col) != '-') col++;
                lines[line] = str.substring(0, col) + ' ' + str.substring(col);
                console.log(str +  ' --> ' + lines[line]);
            });

            file_contents = lines.join('\n');

            // console.log('--preview formatted: ' + file + ' --')
            // console.log(file_contents) // print formatted source
            fs.writeFile(file, file_contents);  // write formatted source
        }       

    });

    console.log('end', json, errorCount)
};

Command

jscs src -c mrdoob.json -r spaces

WestLangley · 2014-05-19T16:40:50Z

Thanks @zz85 !

I also see in path.js

if ( (! holesFirst ) && ( newShapes[mainIdx] ) )

Should be spaces around ! and inside []

if ( ( ! holesFirst ) && ( newShapes[ mainIdx ] ) )

Perhaps there are others that need to be considered, like ~ !! & && | ||, etc...

WestLangley · 2014-05-19T16:42:19Z

Why are the *.Loader files completely replaced?

What happened to cause these files to be completely replaced in this PR?

zz85 · 2014-05-19T17:09:57Z

I'm not too sure ether... They look similar to me, I wonder if its some encoding stuff going on or something?

What happened to cause these files to be completely replaced in this PR?

Good observation... I think requireSpacesInConditionalExpression just landed in jscs and I haven't gotten to test it on our codebase yet. Maybe I should also open a new repository so I can start writing test cases for mrdoob code style.

Perhaps there are others that need to be considered, like ~ !! & && | ||, etc...

For now, I think it might be better for mrdoob to merge bulk of the code style changes if its ok and continue to improve more code style changes, just in case PRs by other users come in and makes it harder for merges.

WestLangley · 2014-05-27T00:54:45Z

@mrdoob How do you feel about this? Apart from my suggestions that @zz85 has not addressed yet, I am in favor of this PR -- unless you feel it is already too late...

mrdoob · 2014-05-28T16:19:31Z

Seems like there are some conflicts again... :/
@zz85 sorry sorry sorry, can you do a new PR with a new pass with the latest in dev?

WestLangley · 2014-05-28T16:36:42Z

@zz85 Can you provide your scripts to @mrdoob so he can run them himself?

Also, this still concerns me:

Why are the *.Loader files completely replaced?

zz85 · 2014-05-31T03:29:37Z

@mrdoob hmm... let me try on Sunday tomorrow. could we have a mini merge freeze so it wouldn't conflict again?

@WestLangley right now its just a whole bunch of assorted scriptfu to get this done, but the process have been documented here. Until autoformatting comes in jscs, this process should be a one-off thing. (jscs has been making some progress lately, so hopefully things like this would be much easier in future).

zz85 · 2014-05-31T03:31:36Z

@WestLangley also, I'm have no idea why *.Loader got replaced. Sometimes I see strange thing like this in git, but I'll take a lookout when I redo this process.

zz85 · 2014-05-31T03:38:16Z

@WestLangley hmm.. I'm trying to do a git diff locally, but it doesn't show up any diffs. I can't tell if its a github issue or not...

zz85 · 2014-05-31T04:28:06Z

@WestLangley hmm... I think this is a CRLF issues. See http://lostechies.com/keithdahlby/2011/04/06/windows-git-tip-hide-carriage-return-in-diff/ I'll try with the git option crlf set to auto this time.

zz85 · 2014-05-31T04:36:27Z

looks like this is the cause

not sure what @mrdoob's take on the CRLF issue...

zz85 · 2014-05-31T04:38:17Z

i'll try not changing line ending encodings for now..

WestLangley · 2014-05-31T04:53:00Z

@zz85 Many thanks for your excellent detective work! : - )

Maybe I should also open a new repository so I can start writing test cases for mrdoob code style.

That is a good backup plan if the process can't be completely automated... Once you file your PR and get most of the formatting as desired, future format changes could be made manually whenever the script identifies ill-formatted lines. (In other words, formatting changes do not have to be completely automated.)

zz85 · 2014-05-31T05:37:04Z

Closing in favour of #4873

zz85 added 2 commits May 13, 2014 10:00

First round at attempt to code transform codebase

305f2df

git reset --hard codepainter xform -j ../utils/codestyle/mrdoob_codepainter.json "*/*.js" find ./ -type f -exec sed -i -e 's/( )/()/g;s/\[ \]/[]/g;s#{[ ]}#\{\}#g' {} \; find . -name "*.js-e*" -print0 | xargs -0 rm git add -p

Add codepainter script

89efa27

zz85 added 2 commits May 20, 2014 00:08

Fix code styles for prefixed unary operators

fa30078

JSCS rules: { "requireSpaceAfterPrefixUnaryOperators": [ "+", "-", "++", "--" ] }

Fix code style spaces before postfix unary operators

69e6d9a

JSCS rules: { "requireSpaceBeforePostfixUnaryOperators": ["++", "--"] }

zz85 added a commit to zz85/three.js that referenced this pull request May 31, 2014

Code transforming mrdoob style second attempt See mrdoob#4806

aafd29c

zz85 mentioned this pull request May 31, 2014

Code Style Improvements 2 #4873

Merged

zz85 closed this May 31, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Style Improvements #4806

Code Style Improvements #4806

zz85 commented May 13, 2014

WestLangley commented May 13, 2014

mrdoob commented May 15, 2014

zz85 commented May 17, 2014

zz85 commented May 19, 2014

WestLangley commented May 19, 2014

WestLangley commented May 19, 2014

zz85 commented May 19, 2014

WestLangley commented May 27, 2014

mrdoob commented May 28, 2014

WestLangley commented May 28, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

WestLangley commented May 31, 2014

zz85 commented May 31, 2014

Code Style Improvements #4806

Code Style Improvements #4806

Conversation

zz85 commented May 13, 2014

WestLangley commented May 13, 2014

mrdoob commented May 15, 2014

zz85 commented May 17, 2014

zz85 commented May 19, 2014

WestLangley commented May 19, 2014

WestLangley commented May 19, 2014

zz85 commented May 19, 2014

WestLangley commented May 27, 2014

mrdoob commented May 28, 2014

WestLangley commented May 28, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

zz85 commented May 31, 2014

WestLangley commented May 31, 2014

zz85 commented May 31, 2014