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

Use the Chart.js shared ESLint config #5112

Merged
merged 1 commit into from Jan 6, 2018

Conversation

simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Jan 6, 2018

An ESLint shareable config has been created (from this repository) in the attempt to homogenize Chart.js hosted projects and plugins style. Rename .eslintrc files to .eslintrc.yml since the ([name has been deprecated(https://eslint.org/docs/user-guide/configuring#configuration-file-formats)) to .eslintrc.yml..

Fix the CC badge (maintainability) and disable CodeClimate ESLint plugin because it doesn't support custom shareable config but also because it already executes relevant checks as part of the regular process.

@simonbrunel
Copy link
Member Author

Well, shareable config doesn't seem supported by CodeClimate (41 fixed issues)

etimberg
etimberg previously approved these changes Jan 6, 2018
@etimberg
Copy link
Member

etimberg commented Jan 6, 2018

Looks like the 41 issues were all either statement count or the complexity. Is it OK to lose those?

@simonbrunel
Copy link
Member Author

I think it actually lints nothing, as if the .eslintrc.yml was empty, so will not report real issues.

@etimberg
Copy link
Member

etimberg commented Jan 6, 2018

Lots of discussion in codeclimate/codeclimate-eslint#86 that may be relevant

@etimberg
Copy link
Member

etimberg commented Jan 6, 2018

Linting will still run for builds & PRs as its part of the CI as well. https://travis-ci.org/chartjs/Chart.js/builds/325810760#L2166

@simonbrunel
Copy link
Member Author

I was reading it but no real solution. The closest is the fetch strategy, but it overrides the existing file which could have custom rules. I think it still great to have CodeClimate reporting complexity and statement count but if we disable ESLint, we will lose these stats.

@etimberg
Copy link
Member

etimberg commented Jan 6, 2018

Maybe there's a codeclimate alternative we could consider? If the cost is low we could self host something, but I'd prefer not to do that if possible

@simonbrunel
Copy link
Member Author

@etimberg I agree with you, we already run ESLint against our code during the build process on Travis and if there are errors, we don't really care about CC result. I think we still want complexity and statement count to be reported by CC because it highlights part of the code that need to be refactored. One solution would be to create a custom .eslintrc.cc.yml in the charts/eslint-config-chartjs repository and fetch it manually.

.eslintrc.cc.yml: enables only checks we want to be reported by CC

rules:
  complexity: [2, 10]
  max-statements: [2, 30]

.codeclimate.yml

prepare:
  fetch:
    - url: 'https://raw.githubusercontent.com/chartjs/eslint-config-chartjs/master/.eslintrc.cc.yml'
      path: '.eslintrc.yml'
engines:
  # ...

@simonbrunel
Copy link
Member Author

It seems to work but actually, CC already have those checks enabled: https://docs.codeclimate.com/v1.0/docs/advanced-configuration#section-default-checks. So maybe the cleanest/simplest is to disable the ESLint CC plugin, what do you think?

@etimberg
Copy link
Member

etimberg commented Jan 6, 2018

That works for me

An ESLint shareable config has been created (from this repository) in the attempt to homogenize Chart.js hosted projects and plugins style. Rename `.eslintrc` files to `.eslintrc.yml` since the name has been deprecated.

Fix the CC badge (maintainability) and disable CodeClimate ESLint plugin because it doesn't support custom shareable config but also because it already executes relevant checks as part of the regular process.
@simonbrunel
Copy link
Member Author

Commits squashed (CC ESLint disabled), I also fixed the CC badge

@etimberg
Copy link
Member

etimberg commented Jan 6, 2018

Looks good to me. Can I merge?

@simonbrunel
Copy link
Member Author

Yes, it needs at least one approval

@etimberg
Copy link
Member

etimberg commented Jan 6, 2018

Will do tonight. Can’t on mobile

@etimberg etimberg merged commit 9ddb449 into chartjs:master Jan 6, 2018
@simonbrunel simonbrunel deleted the shared-eslint-config branch January 6, 2018 23:03
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
An ESLint shareable config has been created (from this repository) in the attempt to homogenize Chart.js hosted projects and plugins style. Rename `.eslintrc` files to `.eslintrc.yml` since the name has been deprecated.

Fix the CC badge (maintainability) and disable CodeClimate ESLint plugin because it doesn't support custom shareable config but also because it already executes relevant checks as part of the regular process.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants