deployd / deployd-cli

The Deployd Command Line Interface
MIT License
7 stars 9 forks source link

Fix eslint issues #33

Closed skathuria29 closed 4 years ago

skathuria29 commented 4 years ago

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:

NicolasRitouet commented 4 years ago

hi @skathuria29 Thanks a lot for your proposal. The build is breaking because it seems that yarn doesn't support node 5. 2 options:

I'm in favor of first option, but I don't want to take the decision by myself. @deployd/developers any opinion?

andreialecu commented 4 years ago

Dropping support for node <6 should be fine, it's quite old by this point anyway. I'm in favor.

skathuria29 commented 4 years ago

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

NicolasRitouet commented 4 years ago

@skathuria29 Since we agreed to get rid of support for node <6, you need to remove this on the config file for travis ci: https://github.com/deployd/deployd-cli/blob/master/.travis.yml The last two lines need to be deleted.

NicolasRitouet commented 4 years ago

@skathuria29 looks great. I've added one comment regarding the name of a variable, but the rest is good. If there's no error anymore in eslint, you should be able to change the eslint rules to errors instead of warning: https://github.com/deployd/deployd-cli/blob/77dd9c257f3bf321eca64ce89b191ef099961800/.eslintrc.js#L16 "no-unused-vars": 1, // 3 errors should be `"no-unused-vars": "error" (same for other rules)

NicolasRitouet commented 4 years ago

hi @skathuria29 looks great! Can you just remove the comments and then, it's good to be merged :)