Feature/eslint #408
Feature/eslint #408
Conversation
.eslintignore
Outdated
| @@ -0,0 +1,13 @@ | |||
| # editorconfig.org | |||
There was a problem hiding this comment.
The reference to jshint should be changed in this section https://github.com/telefonicaid/iotagent-ul/blob/master/docs/usermanual.md#coding-guidelines
(Maybe the same in the other associated PRs)
.editorconfig
Outdated
| # editorconfig.org | ||
| root = true | ||
|
|
||
| [*] |
There was a problem hiding this comment.
Should we force a default style for files not matching any extension? Not sure if it is a good idea to over-force in this case as the potential files can be quite different.
Could be the [*] section safely removed?
There was a problem hiding this comment.
Most of the settings are deliberately general - it would probably make more sense if you tell me which of the setting you don't like and I can remove those.
[*]
indent_style = tab
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true
end_of_line = lf and charset = utf-8 and insert_final_newline = true are standard UNIX defaults and would avoid problems like: #305
trim_trailing_whitespace = true along with insert_final_newline = true align with the linter settings - so trim_trailing_whitespace = true could be moved down and duplicated in the json and js settings.
The only controversial one is indent_style = tab - maybe should probably be redefined for Dockerfile only?
There are a couple of *.sh bash scripts in the repo - which are inconsistently spaced - should they be added?
There was a problem hiding this comment.
Suggested fix: e55fdb6
There was a problem hiding this comment.
The only controversial one is
indent_style = tab- maybe should probably be redefined forDockerfileonly?
Yes, that's controversial... especially when other file formats in the repo are using indent_style = space :). Is there any syntactical requirement that enforce tabs in Dockefiles? Or it's a matter of taste decide between spaces and tabs?
In the second case, I'd suggest to let Dockerfile out of the scope of this file.
There are a couple of
*.shbash scripts in the repo - which are inconsistently spaced - should they be added?
Style & homogeneity for JavaScript source code is great. Style & homogeneity for JSON files could be also great. However, going to far trying to cover each little file of the repository could be excessive... I'd suggest by the moment leaving out *.sh files.
There was a problem hiding this comment.
So something like this: caaffcb
- Default to Unix line endings and UTF-8
- agnostic over tabs/spaces.
- Specific config for JS, md, yaml etc for the things that the linter cares about.
Note that editorConfig is just a suggestion. Husky should run the prettifier to re-enforce code style.
There was a problem hiding this comment.
After the last changes the .eseditorcnofig file looks nice :) Thx!
NTC
lib/bindings/AMQPBinding.js
Outdated
| @@ -190,8 +190,6 @@ function start(callback) { | |||
| numRetried++; | |||
| return setTimeout(createConnection, retryTime * 1000); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
CC: @fiqare-secmotic
Curiously this is contrary to one of the rules from fiQare project, see #411 (comment). We enforce one thing (to remove empty else brarnches) and the opposite (to enforce else branch even when empty) at the same time :)
@jason-fox @fiqare-secmotic what do you think?
There was a problem hiding this comment.
This is an if...else... construct not an if...else if... construct.
Placing the return within the else clause is therefore totally unnecessary.
There was a problem hiding this comment.
Hi! yes, as @jason-fox says, it is not an if...else if construct
There was a problem hiding this comment.
Just to check if I'm understanding it correctly...
The MISRA and CERT rules cited at #411 (comment), prescribe the inclusion of final "else" in "if... else if..." structures but not prescribe the use of "else" in simple "if" structured.
@fiqare-secmotic, could you confirm my understanding is correct?
There was a problem hiding this comment.
@fiqare-secmotic, could you confirm my understanding is correct?
Any news?
There was a problem hiding this comment.
@fgalan yes, your understanding is correct.
jason-fox
force-pushed
the
feature/eslint
branch
from
caaffcb to
7366b72
Compare
|
Should I understand this PR has moved to PR #415 ? Why? (it's fine, just curiosity...) The question raised at #408 (comment) still having no answer. @fiqare-secmotic could you have a look, please? Thanks! |
|
@fgalan - I was just doing some automated scripted housekeeping and instead of updating |
Update codebase to use ES6