Fix eslint issues #33
Conversation
|
hi @skathuria29
I'm in favor of first option, but I don't want to take the decision by myself. |
|
Dropping support for node <6 should be fine, it's quite old by this point anyway. I'm in favor. |
|
Ohh I thought the build was failing because of my changes. This was my first time contributing to an open source project. Please review the changes and let me know if it requires any further modifications. Thanks and I would be happy to contribute more 😄 @NicolasRitouet |
|
@skathuria29 |
lib/cli/create.js
Outdated
| name = name || 'my-deployd-app'; | ||
| if (test('-d', name)) { | ||
| return console.info(`${name} already exists in this directory`); | ||
| const newName = name || 'my-deployd-app'; |
There was a problem hiding this comment.
naming a variable new is not recommended because something new at the time of writing won't probably stay new for a long time. (I remember an api I encountered at a customer called newCustomerApi built in 2003... Not so new anymore, but still around).
In this particular case, I'd rather change the name of the param with something like _name and keep the name of the variable used in the function name. It makes reading the code in the function easier (remember, when writing code, you want to reduce cognitive complexity
const create = function (_name) {
const name= _name || 'my-deployd-app';
There was a problem hiding this comment.
Yeah that makes sense. Please review the changes I've made with respect to renaming the variable and changing the warnings to error in eslintrc.
I've kept the import/no-dynamic-require in /.eslintrc.js as a warning as I wasn't able to fix it.
|
@skathuria29 looks great. Line 16 in 77dd9c2 "no-unused-vars": 1, // 3 errors should be `"no-unused-vars": "error" (same for other rules)
|
.eslintrc.js
Outdated
| "consistent-return": 1, // 8 errors | ||
| "no-param-reassign": 1, // 2 errors | ||
| "global-require": 1, // 2 errors | ||
| "prefer-const": "error", // 4 errors |
There was a problem hiding this comment.
can you also remove the comments (on each line) as they are not accurate anymore?
|
hi @skathuria29 looks great! |
Description
Fixed eslint global-require rule - did changes in 2 files: keygen.js and showkey.js.
Fixed eslint no-param-reassign rule - it required changes in: create.js and keys.js.
Fixed eslint no-unused-vars rule - required changes in create.js, index.js and start.js.
Fixed eslint one-var and prefer-const rule - by changing start.js.
Fixed eslint no-use-before-define rule - did changes in start.js.
Fixed eslint consistent-return rule - required changes in the following files keygen.js, showkey.js, start.js and keys.js.
Motivation and Context
Fixed eslint errors #16
How Has This Been Tested?
npm run test command
Screenshots (if appropriate):
Types of changes
Checklist: