[qa] Fix linting erros #112 #121
Conversation
There was a problem hiding this comment.
Please make the linting rules similar to https://github.com/openwisp/openwisp-wifi-login-pages to maintain consitency across the different openwisp JS modules.
We need also a CI build, for that I created #123.
|
I'm on it. |
There was a problem hiding this comment.
Good progress @totallynotvaishnav! 👍
Don't we need .babelrc too? It may not have to be exactly as wifi-login-pages but I think having it similar should be good.
Also, what about .prettierrc and .prettierignore?
.eslintrc.js
Outdated
| 'no-underscore-dangle': 'off', | ||
| 'no-plusplus': 'off', | ||
| 'no-param-reassign': 'off', | ||
| 'class-methods-use-this': 'off', |
There was a problem hiding this comment.
Why these? Are we sure these are good? It doesn't look so good, are these needed because there's some code which needs this? If yes, please create a separate issue to fix it.
There was a problem hiding this comment.
Right now, pretty much most of the code violates these rules. To fix these we will have to rewrite most of the code.
There was a problem hiding this comment.
Please open an issue for this so we can analyze it, it's not great to have bad practices allowed.
There was a problem hiding this comment.
for the moment turn the rules so that the build passes and we can work on the changes required to have these rules off when working on the dedicated issue. Right now the priority is to have the build running fine so it's a first checkpoint for any review and will make our work easier (especially for mentors).
| // : _this.utils.nodeInfo(params.data.node); | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Interesting! I think we didn't see this commented code here before, could you open a new issue to fix this? Showing info on hover is pretty importantin some cases.
There was a problem hiding this comment.
Sure I will open an issue.
I will add it in #123. |
totallynotvaishnav
force-pushed
the
issues/112-fix-linting
branch
from
6a90420
to
70a9705
Compare
|
#129 needs to be merged before this. Maybe we can do both tasks in this PR. |
totallynotvaishnav
force-pushed
the
issues/112-fix-linting
branch
from
70a9705
to
f655f72
Compare
.eslintrc.js
Outdated
| 'no-underscore-dangle': 'off', | ||
| 'no-plusplus': 'off', | ||
| 'no-param-reassign': 'off', | ||
| 'class-methods-use-this': 'off', |
There was a problem hiding this comment.
for the moment turn the rules so that the build passes and we can work on the changes required to have these rules off when working on the dedicated issue. Right now the priority is to have the build running fine so it's a first checkpoint for any review and will make our work easier (especially for mentors).
| @@ -39,23 +38,24 @@ class NetJSONGraphUpdate extends NetJSONGraphUtil { | |||
| } | |||
| }; | |||
|
|
|||
| // eslint-disable-next-line consistent-return | |||
There was a problem hiding this comment.
I've disabled some rules locally for the build to pass.
closes #112