Skip to content

CLI: Linter fixes#943

Merged
mnaamani merged 5 commits intoJoystream:nicaeafrom
Lezek123:cli-linter-fixes
Jul 16, 2020
Merged

CLI: Linter fixes#943
mnaamani merged 5 commits intoJoystream:nicaeafrom
Lezek123:cli-linter-fixes

Conversation

@Lezek123
Copy link
Contributor

@Lezek123 Lezek123 commented Jul 14, 2020

Introduces fixes to all linter errors following #917

@Lezek123 Lezek123 marked this pull request as draft July 14, 2020 14:47
Leszek Wiesner added 2 commits July 15, 2020 07:13
@Lezek123 Lezek123 marked this pull request as ready for review July 15, 2020 05:47
@Lezek123 Lezek123 requested a review from mnaamani July 15, 2020 05:47
Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Can you also run yarn format and check-in the changed files.
The reason the --fix argument when you ran linter didn't sort those out is because the lint script is only linting .ts files /src folder I think.

Perhaps update it the lint script to cover the whole directory. So format, lint, and checks all cover the same files.

Perhaps also update .github/workflows/joystream-cli.yml

to run yarn workspace joystream-cli checks in place of yarn workspace joystream-cli build

@mnaamani
Copy link
Member

I think ideally

    "lint": "eslint . --quiet --ext .ts",
    "checks": "yarn lint && tsc --noEmit --pretty && prettier . --check",
    "format": "prettier . --write"

but will need to fix:

/Users/mokhtar/joystream/joystream/cli/test/commands/council/info.test.ts
  3:1  error  'describe' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

I think adding mocha to the env in the eslintrc file should do the trick

@Lezek123
Copy link
Contributor Author

Lezek123 commented Jul 15, 2020

Fixed the mentioned issues.
While testing I noticed the lack of unused imports check for the CLI.
I found this check very helpful in Pioneer. so I added @typescript-eslint/no-unused-vars rule to CLI's .eslintrc.js too. Also set "noUnusedLocals": true in tsconfig.json (the latter will additionaly cause the build to fail if there are any unused imports spotted).

Perhaps this can be part of the common standard?

Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the mentioned issues.
While testing I noticed the lack of unused imports check for the CLI.
I found this check very helpful in Pioneer. so I added @typescript-eslint/no-unused-vars rule to CLI's .eslintrc.js too. Also set "noUnusedLocals": true in tsconfig.json (the latter will additionaly cause the build to fail if there are any unused imports spotted).

Perhaps this can be part of the common standard?

Yes those rules look very good and can catch a tonne of bugs. I'm surprised the no-unused-vars isn't on by default in the rule-sets we have.

Added reminder in #664

@mnaamani mnaamani merged commit e6325a6 into Joystream:nicaea Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants