Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cypress eslint fixes and config #3636

Merged
merged 10 commits into from
Mar 25, 2019
Merged

Cypress eslint fixes and config #3636

merged 10 commits into from
Mar 25, 2019

Conversation

ranbena
Copy link
Contributor

@ranbena ranbena commented Mar 24, 2019

What type of PR is this?

  • Feature

Description

  1. Added cypress folder to lint script.
    • updated npm run lint to run on ./client instead of ./client/app.
    • added **/dist to .eslintignore
    • apparently eslint never detected the ignore file cause it's not in project root 😵 so added --ignore-path ./client/.eslintignore. Now there's also no need for lint:staged specific folder ignoring.
    • removed node_modules from ignore file cause it's ignored by default.
  2. Cypress specific eslint config
    • Created eslintrc in cypress folder (eslint cascading feature).
    • Removed cypress stuff from main eslintrc.
    • Added cypress lint plugin that enforces cypress best practices.
    • Allowing chai assertions like expect(element).to.be(true).
    • Moved jest plugin to ./client/app cause it collided with chai syntax.
  3. Fixed lint errors and warnings
    • fixed some code
    • disabled lint where necessary
    • disabled func-name lint to allow anonymous functions it('test', function () {.

Tested on npm run lint, lint-staged and vscode.

@ranbena ranbena requested review from gabrieldutra and arikfr March 24, 2019 19:17
@ranbena ranbena force-pushed the cypress-eslint branch 2 times, most recently from f0f3e29 to 5ef1cbe Compare March 24, 2019 19:56
@ranbena
Copy link
Contributor Author

ranbena commented Mar 24, 2019

Looks like codeclimate complies with the root eslintrc but ignores the rest 😩
Could it be that this line is revoking the cascading feature?:

config: client/.eslintrc.js

Might have been said here.

@arikfr @gabrieldutra wdyt?

@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

Perhaps we can do linting from a CircleCI step instead of CodeClimate?

@arikfr
Copy link
Member

arikfr commented Mar 25, 2019

The issue you linked to mentions:

We do support cascading .eslintrc* files in effectively the same way that vanilla ESLint supports them. Does that clear it up a bit?

Maybe we need to remove the explicit eslint config setting and let it find on its own?

@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

lol it worked

@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

I'm gonna test it by pushing a lint error.

@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

No good. There should be 2 errors showing up, but CodeClimate is oblivious @arikfr

@arikfr
Copy link
Member

arikfr commented Mar 25, 2019

It seems to ignore some plugins.

@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

I see 3 ways to go about this:

  1. Understand and fix CodeClimate config.
  2. Switch to CircleCi step for linting (not tested).
  3. Merge all rules into the main eslintrc.

Gonna try #3 now.

@ranbena
Copy link
Contributor Author

ranbena commented Mar 25, 2019

@arikfr so I narrowed it down to a working config:

  1. Back to one ./client/eslintrc.js.
  2. Can't use plugin:cypress/recommended - causes CodeClimate error. Helpful lints but not a must.

Good to go?

@@ -43,6 +43,7 @@ function addQueryByAPI(data, shouldPublish = true) {
schedule: null,
}, data);

// eslint-disable-next-line cypress/no-assigning-return-values
Copy link
Member

Choose a reason for hiding this comment

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

cypress/recommended seemed nice 😕. As we can't use it, should those cypress eslint-disable be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's cause the cypress package isn't a required installation.

Copy link
Member

Choose a reason for hiding this comment

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

Might be 🤔

@arikfr
Copy link
Member

arikfr commented Mar 25, 2019

Good to go?

While I liked the fact you split the configuration, the current version is better than what we had until now so good to go. 👍

@ranbena ranbena merged commit ec4b36b into master Mar 25, 2019
@ranbena ranbena deleted the cypress-eslint branch March 25, 2019 20:14
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
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.

3 participants