Skip to content
This repository has been archived by the owner on Mar 2, 2019. It is now read-only.

Added JSCS & JSHint with Travis CI config #317

Closed
wants to merge 17 commits into from
Closed

Added JSCS & JSHint with Travis CI config #317

wants to merge 17 commits into from

Conversation

ShashankaNataraj
Copy link
Contributor

This PR fixes #314

  • .jshintrc
    • Put brakes { after rule declarations/function name/while/for/etc. curly:true
  • .jscsrc (Each completed task here has the corresponding jshint rule listed in code blocks)
    • Use two space indent "validateIndentation": 2
    • Put spaces between rule declarations/function name/while/for/etc and brakes {.requireSpacesInAnonymousFunctionExpression,requireSpacesInCallExpression,requireSpacesInConditionalExpression,requireSpacesInForStatement,requireSpacesInFunctionDeclaration,requireSpacesInFunctionExpression,requireSpacesInFunction,requireSpacesInNamedFunctionExpression
    • Put spaces after : in property declarations/objects definitions. "requireSpaceAfterObjectKeys": true
    • Use Spaces after and before ( and ) and [ and ]. "requireSpacesInsideParentheses":"all","requireSpacesInsideArrayBrackets": "all"
    • Use Spaces after and before = assigning values. "requireSpaceAfterBinaryOperators":["="],"requireSpaceBeforeBinaryOperators":["="]
    • Don't use spaces after unary operators such as ! or ++. "disallowSpaceAfterPrefixUnaryOperators":["++", "--", "+", "-", "~", "!"]
    • Use space after , in arguments variable. "validateParameterSeparator":", "
    • Declaring Objects using multi-line. "requireMultipleVarDecl":true
    • Declaring variables using multi-line. "requireMultipleVarDecl":true
    • Always use semicolons ;. "requireSemicolons":true
    • put spaces after double slash. ("requireSpaceAfterLineComment":true)
    • Any , must not have preceding space "disallowSpaceAfterBinaryOperators":[","],
    • Use $ in front of a variable if it's a jquery object. requireDollarBeforejQueryAssignment:true
    • General guidelines
      • "disallowMixedSpacesAndTabs": true
      • "disallowMultipleSpaces":true
      • "disallowTrailingComma":true
      • "requireLineFeedAtFileEnd":true

Style guidelines which do not have jshint / jscsc equivalents:

@ShashankaNataraj
Copy link
Contributor Author

@arthurvr @grayghostvisuals @benschwarz @wellingguzman Please review 🎱

@arthurvr
Copy link
Member

@arthurvr @grayghostvisuals @benschwarz @wellingguzman Please review 🎱

No real need for such comments. We're all busy and will try to get to this as soon as possible.

@@ -0,0 +1,7 @@
language: node_js
node_js:
- "0.10"

This comment was marked as abuse.

This comment was marked as abuse.

@ShashankaNataraj
Copy link
Contributor Author

@arthurvr no offense intended with the mention. Will make changes.

@ShashankaNataraj
Copy link
Contributor Author

Heres a new checklist of action after review

  • .travis.yml
    • Update NodeJS version in .travis.yml
    • Change .travis.yml to invoke npm test instead of grunt quality
  • Gruntfile.js
    • jshint:{ and jscs:{ require spaces after the colon
    • Lint the gruntfile and fix lint errors
    • grunt.registerTask('quality', ['jshint:all', 'jscs:all']); is missing spaces
    • jscs and jshint can be removed from packages.json as only the grunt tasks are required
    • Update NodeJS version

@arthurvr
Copy link
Member

Also please squash your commits.


// Load NPM Tasks
// https://github.com/shootaroo/jit-grunt
require('jit-grunt')(grunt);
require ( 'jit-grunt' ) ( grunt );

This comment was marked as abuse.

@arthurvr
Copy link
Member

Should we consider dropping the current JavaScript style guide and going with an already-existing style guide? It's something else we don't have to maintain and honestly ours is pretty ugly and incomplete. Now is the right time to discuss it.

@arthurvr
Copy link
Member

We also need a Travis CI badge in the readme.

@ShashankaNataraj
Copy link
Contributor Author

@arthurvr I guess the Travis CI Badge can only be added by one of the core contributors on h5bp, since if anyone else adds it itll point to their builds.

I will wait for further discussions before proceeding further regarding the code style.

@arthurvr
Copy link
Member

I guess the Travis CI Badge can only be added by one of the core contributors on h5bp, since if anyone else adds it itll point to their builds.

No, you can perfectly just edit the link address to point to our repo :p

@ShashankaNataraj
Copy link
Contributor Author

ok :) will do. In general isn't jQuery's coding style considered pretty mature?

@arthurvr
Copy link
Member

In general isn't jQuery's coding style considered pretty mature?

Define "mature"?

jQuery's code style guide is pretty ugly in my opinion but it's to the @h5bp/effeckt-css team to decide this.

@ShashankaNataraj
Copy link
Contributor Author

mature is generally accepted and followed widely?

@wellingguzman
Copy link
Member

We can reestablish the code guideline, there's probably are some not common things there, since I wrote what I've used by that time as the guideline.

Please you should squash those commits, Getting-Started Wiki so that way would be easier to git bisect, or track changes.

@ShashankaNataraj
Copy link
Contributor Author

@wellingguzman Will squash my commits when I get time later today :)

@@ -2,6 +2,7 @@
/.sass-cache/
dist
node_modules/
.idea/

This comment was marked as abuse.

@arthurvr
Copy link
Member

We can reestablish the code guideline, there's probably are some not common things there, since I wrote what I've used by that time as the guideline.

Yeah, we should really revision it.

jscs: {
demo:[ 'js/demo/*.js' ],
modules:[ 'js/modules/*.js' ],
all:[ 'js/**/*.js' ]

This comment was marked as abuse.

@ShashankaNataraj
Copy link
Contributor Author

Can we revision the code guideline first and then revise this PR with changes? Because it looks like efforts on this PR will not be complete without the code guidelines being revised.

@arthurvr
Copy link
Member

Can we revision the code guideline first and then revise this PR with changes? Because it looks like efforts on this PR will not be complete without the code guidelines being revised.

We can always land the grunt configs and discussion revision of the style guide later on.

@ShashankaNataraj
Copy link
Contributor Author

ok I will be doing the grunt and the JSCS config first and raising a PR.

@Romainpetit
Copy link

@ShashankaNataraj Is this ever going to be completed ?

@ShashankaNataraj
Copy link
Contributor Author

@Romainpetit Don't think so my friend. I lost interest in this a long time back

@naeluh
Copy link

naeluh commented Mar 31, 2017

@ShashankaNataraj What needs to be completed , I be happy to work on this ?

@ShashankaNataraj
Copy link
Contributor Author

@naeluh @Romainpetit Snarky comments from the maintainers of this project put me off from completing this PR. Now, Im going to unsubscribe from this thread and let this PR rot.

Aidos.

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

Successfully merging this pull request may close these issues.

Add JSHint and JSCS
6 participants