Skip to content
This repository has been archived by the owner on Aug 6, 2021. It is now read-only.

Added eslint-tests based on sane and fixed codebase. #32

Merged
merged 1 commit into from
Apr 23, 2015

Conversation

Globegitter
Copy link
Contributor

We have been using eslint quite succesfully now for a while over at sane and I also just released https://github.com/Globegitter/sails-hook-eslint which detects 'errors' in the blueprints.

Since I think it is a generally nice thing to have and it would be great to have less of them I added it as a simple test, run via npm test. I took the styles exactly from sane-cli and you can find it in the .eslintrc.

These are the errors it throws on the codebase before this PR (excluding blueprints, which have over 600)

bin/index.js
  22:34  error  Unexpected trailing comma        comma-dangle
  26:2   error  Expected { after 'if' condition  curly

 2 problems (2 errors, 0 warnings)



  2) eslint should have no errors in lib:
     Error: Code did not pass lint rules
lib/before.js
  5:4  error  util is defined but never used  no-unused-vars

lib/index.js
  21:20   error  Missing space before function parentheses    space-before-function-parentheses
  29:32   error  Strings must use singlequote                 quotes
  36:8    error  Keyword "if" must be followed by whitespace  space-after-keywords
  36:12   error  Unexpected space after unary operator "!"    space-unary-ops
  40:87   error  Missing space before function parentheses    space-before-function-parentheses
  41:84   error  Strings must use singlequote                 quotes
  41:172  error  Strings must use singlequote                 quotes
  41:205  error  Strings must use singlequote                 quotes
  44:68   error  Strings must use singlequote                 quotes
  44:156  error  Strings must use singlequote                 quotes
  50:5    error  Unexpected trailing comma                    comma-dangle

 12 problems (12 errors, 0 warnings)



  3) eslint should have no errors in tests:
     Error: Code did not pass lint rules
tests/runner.js
  33:4  error  Don't use process.exit(); throw an error instead  no-process-exit

✖ 1 problem (1 error, 0 warnings)

What do you think @mphasize Do you like all these rules? If so I would get started on fixing the blueprints.

@mphasize
Copy link
Owner

@Globegitter Sorry that I've been so silent recently. I absolutely favor the idea of adding tests. Let me take a look at the tests and rules now...

@mphasize
Copy link
Owner

Nice stuff and I like the rules you selected. Concerning adapting the blueprints to follow those rules: Not super sure. I took the original Sails blueprints and tried to keep modifications to a minimum, which means that I can now easily spot changes in new Sails versions by doing a simple diff. That would get harder, when we change the blueprints.
On the other hand, I think the blueprint code has been a bit messy in the first place and it would probably benefit from a makeover.
Has anybody heard if Sails might adapt some kind of strict code styling rules in the future?

mphasize added a commit that referenced this pull request Apr 23, 2015
Added eslint-tests based on sane and fixed codebase.
@mphasize mphasize merged commit ef67b19 into master Apr 23, 2015
@Globegitter
Copy link
Contributor Author

@mphasize there is a PR on waterline: balderdashy/waterline#935 I should probably split the changes up into two PRs, but the idea was bring it to waterline first and then move it to sails. Would be great to hear some more opinions on the PR itself.

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.

None yet

2 participants