SunsetRiders / nodejs-scaffold

A Node.js scaffolding for a web client with Express.js
0 stars 1 forks source link

Replacing JSCS with ESLint #9

Closed gvillalta99 closed 8 years ago

gvillalta99 commented 8 years ago
gvillalta99 commented 8 years ago

@msughawara can you review this PR?

msughawara commented 8 years ago

I would like to review the two scenarios in git worflow: 1) the general related to PR 2) the specific one related to the code.

gvillalta99 commented 8 years ago

Ok @msughawara,

First of all, thank you for reviewing my PR. I will brief you about what this change is about, so we can cover the first scenario.

So, we were looking for a tool that could help us to maintain a style on our code base and @jmcoimbra told us that he used a tool called JSCS that aimed to do that on a different project. Because of his experience we picked that tool and while I was searching on how to configure I find out that JSCS was merged into ESLint and discontinued.

That news lead me to talk about replacing JSCS by ESLint with @jmcoimbra and @habner and they all agreed.

I hope this helps you clarifies any doubts about this PR intents. That is anything else that I can do to help you? Do you have any other suggestions?

gvillalta99 commented 8 years ago

Running gulp lint

Before

[11:35:13] Using gulpfile ~/src/nodejs-scaffold/gulpfile.js
[11:35:13] Starting 'jshint'...
[11:35:13] Starting 'jscs'...

gulpfile.js
  line 7  col 5  'buffer' is defined but never used.
  line 8  col 5  'uglify' is defined but never used.

  ⚠  2 warnings

Illegal space after opening curly brace at /home/dev/src/nodejs-scaffold/server.js :
    37 |app.use(logger('dev'));
    38 |app.use(bodyParser.json());
    39 |app.use(bodyParser.urlencoded({ extended: false }));
---------------------------------------^
    40 |app.use(expressValidator());
    41 |app.use(methodOverride('_method'));

Illegal space before closing curly brace at /home/dev/src/nodejs-scaffold/server.js :
    37 |app.use(logger('dev'));
    38 |app.use(bodyParser.json());
    39 |app.use(bodyParser.urlencoded({ extended: false }));
-------------------------------------------------------^
    40 |app.use(expressValidator());
    41 |app.use(methodOverride('_method'));

Illegal space after opening curly brace at /home/dev/src/nodejs-scaffold/server.js :
    40 |app.use(expressValidator());
    41 |app.use(methodOverride('_method'));
    42 |app.use(session({ secret: process.env.SESSION_SECRET, resave: true, saveUninitialized: true }));
-------------------------^
    43 |app.use(flash());
    44 |app.use(express.static(path.join(__dirname, 'public')));

Illegal space before closing curly brace at /home/dev/src/nodejs-scaffold/server.js :
    40 |app.use(expressValidator());
    41 |app.use(methodOverride('_method'));
    42 |app.use(session({ secret: process.env.SESSION_SECRET, resave: true, saveUninitialized: true }));
---------------------------------------------------------------------------------------------------^
    43 |app.use(flash());
    44 |app.use(express.static(path.join(__dirname, 'public')));

4 code style errors found.
Illegal space after opening curly brace at /home/dev/src/nodejs-scaffold/controllers/contact.js :
    25 |  req.assert('email', 'Email cannot be blank').notEmpty();
    26 |  req.assert('message', 'Message cannot be blank').notEmpty();
    27 |  req.sanitize('email').normalizeEmail({ remove_dots: false });
------------------------------------------------^
    28 |
    29 |  var errors = req.validationErrors();

Illegal space before closing curly brace at /home/dev/src/nodejs-scaffold/controllers/contact.js :
    25 |  req.assert('email', 'Email cannot be blank').notEmpty();
    26 |  req.assert('message', 'Message cannot be blank').notEmpty();
    27 |  req.sanitize('email').normalizeEmail({ remove_dots: false });
-------------------------------------------------------------------^
    28 |
    29 |  var errors = req.validationErrors();

Operator + should not stick to preceding expression at /home/dev/src/nodejs-scaffold/controllers/contact.js :
    35 |
    36 |  var mailOptions = {
    37 |    from: req.body.name + ' ' + '<'+ req.body.email + '>',
-------------------------------------------^
    38 |    to: 'your@email.com',
    39 |    subject: '✔ Contact Form | Mega Boilerplate',

Illegal space after opening curly brace at /home/dev/src/nodejs-scaffold/controllers/contact.js :
    42 |
    43 |  transporter.sendMail(mailOptions, function(err) {
    44 |    req.flash('success', { msg: 'Thank you! Your feedback has been submitted.' });
----------------------------------^
    45 |    res.redirect('/contact');
    46 |  });

Illegal space before closing curly brace at /home/dev/src/nodejs-scaffold/controllers/contact.js :
    42 |
    43 |  transporter.sendMail(mailOptions, function(err) {
    44 |    req.flash('success', { msg: 'Thank you! Your feedback has been submitted.' });
--------------------------------------------------------------------------------------^
    45 |    res.redirect('/contact');
    46 |  });

5 code style errors found.

controllers/contact.js
  line 43  col 46  'err' is defined but never used.

  ⚠  1 warning

Expected indentation of 0 characters at /home/dev/src/nodejs-scaffold/public/js/main.js :
     1 |(function() {
     2 |
     3 |  // Your custom JavaScript goes here
--------^
     4 |
     5 |})();

1 code style error found.
[11:35:21] Finished 'jscs' after 7.97 s
[11:35:21] Finished 'jshint' after 7.99 s
[11:35:21] Starting 'lint'...
[11:35:21] Finished 'lint' after 14 μs

After

[11:37:24] Using gulpfile ~/src/nodejs-scaffold/gulpfile.js
[11:37:24] Starting 'jshint'...
[11:37:24] Starting 'eslint'...

gulpfile.js
  line 7  col 5  'buffer' is defined but never used.
  line 8  col 5  'uglify' is defined but never used.

  ⚠  2 warnings

controllers/contact.js
  line 43  col 46  'err' is defined but never used.

  ⚠  1 warning

[11:37:27] 
/home/dev/src/nodejs-scaffold/gulpfile.js
  7:5  error  'buffer' is defined but never used  no-unused-vars
  8:5  error  'uglify' is defined but never used  no-unused-vars

/home/dev/src/nodejs-scaffold/server.js
   2:1   error    Expected space or tab after '//' in comment    spaced-comment
  15:1   error    Expected space or tab after '//' in comment    spaced-comment
  39:31  error    There should be no space after '{'             object-curly-spacing
  39:49  error    There should be no space before '}'            object-curly-spacing
  42:1   warning  Line 42 exceeds the maximum line length of 80  max-len
  42:17  error    There should be no space after '{'             object-curly-spacing
  42:93  error    There should be no space before '}'            object-curly-spacing
  52:33  error    Expected space or tab after '/*' in comment    spaced-comment

/home/dev/src/nodejs-scaffold/controllers/contact.js
  10:1   error    Missing JSDoc for parameter 'res'              valid-jsdoc
  10:1   error    Missing JSDoc for parameter 'req'              valid-jsdoc
  19:1   error    Missing JSDoc @return for function             valid-jsdoc
  19:1   error    Missing JSDoc for parameter 'req'              valid-jsdoc
  19:1   error    Missing JSDoc for parameter 'res'              valid-jsdoc
  27:40  error    There should be no space after '{'             object-curly-spacing
  27:42  error    Identifier 'remove_dots' is not in camel case  camelcase
  27:61  error    There should be no space before '}'            object-curly-spacing
  37:31  error    Unexpected string concatenation of literals    no-useless-concat
  37:36  error    Infix operators must be spaced                 space-infix-ops
  43:37  warning  Expected error to be handled                   handle-callback-err
  43:46  error    'err' is defined but never used                no-unused-vars
  44:1   warning  Line 44 exceeds the maximum line length of 80  max-len
  44:26  error    There should be no space after '{'             object-curly-spacing
  44:80  error    There should be no space before '}'            object-curly-spacing

/home/dev/src/nodejs-scaffold/controllers/home.js
  1:1  error  Missing JSDoc for parameter 'req'  valid-jsdoc
  1:1  error  Missing JSDoc for parameter 'res'  valid-jsdoc

/home/dev/src/nodejs-scaffold/test/app.test.js
   4:1  error  'describe' is not defined  no-undef
   5:3  error  'it' is not defined        no-undef
  12:1  error  'describe' is not defined  no-undef
  13:3  error  'it' is not defined        no-undef

✖ 31 problems (28 errors, 3 warnings)

[11:37:27] 'eslint' errored after 3.24 s
[11:37:27] ESLintError in plugin 'gulp-eslint'
Message:
    Failed with 28 errors
[11:37:32] Finished 'jshint' after 7.95 s

@msughawara this are the changes on the output of the gulp lint command. Basically the new version is more compact, but that is it, they all get the same errors.

gvillalta99 commented 8 years ago

@msughawara please, let me know when I can merge this

jmcoimbra commented 8 years ago

Let @msughawara have this opportunity. @gvillalta99 thanks for your help!

msughawara commented 8 years ago

Those set up changes from JSCS to ESLint are OK. Please add those comments you previously made in README.md in order to document the purpose of this change.

gvillalta99 commented 8 years ago

@msughawara as we talked with @jmcoimbra, let's keep those changes only on git and github to keep it as simple as possible.