Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

Source code formatting #32

Merged
merged 2 commits into from
Dec 17, 2014
Merged

Source code formatting #32

merged 2 commits into from
Dec 17, 2014

Conversation

x1ddos
Copy link
Contributor

@x1ddos x1ddos commented Dec 16, 2014

  • .editorconfig settings
  • new gulp jscs task to check JS code style
  • both jshint and jscs are now being run before default build task
  • pre-commit hook to check code format before committing to master

Closes #31


I tried to match jscs rules to the current source tree format as much as possible but gulp jscs still fails. These need to be fixed for the next build to succeed:

$ gulp jscs
[23:29:43] Using gulpfile ~/p/ioweb2015/gulpfile.js
[23:29:43] Starting 'jscs'...
[23:29:43] 'jscs' errored after 321 ms
[23:29:43] Error in plugin 'gulp-jscs'
Message:
    Expected indentation of 2 characters at main.js :
    17 |(function(exports) {
    18 |
    19 |'use strict';
----------^
    20 |
    21 |// @codekit-prepend 'third_party/signals.min.js'

Expected indentation of 2 characters at main.js :
    22 |// @codekit-prepend 'third_party/requestAnimationFrame.js'
    23 |
    24 |exports.CDS = {};
----------^
    25 |
    26 |// @codekit-append 'helper/event-publisher.js'

Multiple line break at components/button.js :
    85 |    }
    86 |  }
    87 |
--------^
    88 |
    89 |})();

Extra quotes for key at components/schedule.js :
    56 |
    57 |  var days = [{
    58 |    "Breakfast": [{
------------^
    59 |      start: 8, duration: 1
    60 |    }],

Extra quotes for key at components/schedule.js :
    59 |      start: 8, duration: 1
    60 |    }],
    61 |    "Keynote": [{
------------^
    62 |      start: 9, duration: 0.5
    63 |    },{

Extra quotes for key at components/schedule.js :
    64 |      start: 17, duration: 0.25
    65 |    }],
    66 |    "Sessions": [{
------------^
    67 |      start: 9.5, duration: 1.5
    68 |    }, {

Extra quotes for key at components/schedule.js :
    73 |      start: 16.5, duration: 0.5
    74 |    }],
    75 |    "Break": [{
------------^
    76 |      start: 11, duration: 0.5
    77 |    },{

Extra quotes for key at components/schedule.js :
    85 |  },
    86 |  {
    87 |    "Breakfast": [{
------------^
    88 |      start: 8, duration: 1.5
    89 |    }],

Extra quotes for key at components/schedule.js :
    88 |      start: 8, duration: 1.5
    89 |    }],
    90 |    "Sessions": [{
------------^
    91 |      start: 9.5, duration: 2
    92 |    },{

Extra quotes for key at components/schedule.js :
    98 |      start: 16.5, duration: 1.5
    99 |    }],
   100 |    "Break": [{
------------^
   101 |      start: 11.5, duration: 0.5
   102 |    },{

Multiple line break at helper/event-publisher.js :
    90 |    remove: remove
    91 |  };
    92 |
--------^
    93 |
    94 |})();

* .editorconfig settings
* new `gulp jscs` task to check JS code style
* both `jshint` and `jscs` are now being run before `default` build task
* pre-commit hook to check code format before committing to `master`

Closes #31
@@ -0,0 +1,18 @@
#!/bin/bash

if [ "$(git rev-parse --abbrev-ref HEAD)" != "master" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but we're only checking code into branches and submitting PRs. Is there a way we can modify this to be useful for that case too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case you probably don't want it to run all the times, when intentionally committing a WIP code.
We could change the trigger so that it runs only if a commit message contains something like "pre-submit check", but you'll have to remember it.

Normally, for merging PR, this would be a job of a CI. Like Travis adding its info above the "merge" button so that you know it passed all checks/tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so this hook is just a precaution for now, for a rare case where you happened to be on the master branch for some reason.

@ebidel
Copy link
Contributor

ebidel commented Dec 17, 2014

Nice. Seems to mostly follow G's style guide. Comments about master.

@x1ddos
Copy link
Contributor Author

x1ddos commented Dec 17, 2014

Re: master. I was thinking about setting up a private CI somewhere, so that at least gulp jshint and gulp jscs were run before merging a PR.

@ebidel
Copy link
Contributor

ebidel commented Dec 17, 2014

LGTM. For now, I think we can live with just master. We can't put up a CI because the repo is private for now.

ebidel added a commit that referenced this pull request Dec 17, 2014
@ebidel ebidel merged commit 0921a9b into master Dec 17, 2014
@ebidel ebidel deleted the codestyle branch December 17, 2014 01:39
@ebidel ebidel mentioned this pull request Dec 17, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editing/coding style: investigate gulp-jscs and .editorconfig
2 participants