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

Update eslint and eslint-config-cesium #11996

Merged
merged 7 commits into from
May 28, 2024
Merged

Update eslint and eslint-config-cesium #11996

merged 7 commits into from
May 28, 2024

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented May 21, 2024

Description

Following up from CesiumGS/eslint-config-cesium#6, this bumps the version of eslint and corresponding packages. The major change is a migration from .eslintrc.js files to flat config files.

Other notable changes include:

  • Added a package.json and corresponding config to .github/actions/check-for-cla. This was mainly to make eslint happy, but is probably better practice anyway to ensure version compatibility with dependencies.
  • Removed outdated inline eslint comments where found
  • Bumped minimum supported version for compatibility, which matches the current LTS of NodeJS anyway

Issue number and link

N/A

Testing plan

  1. Run npm eslint and confirm there are no issues
  2. Introduce an eslint error to ensure that errors still surface when running npm eslint.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • [-] I have updated CHANGES.md with a short summary of my change
  • [-] I have added or updated unit tests to ensure consistent code coverage
  • [-] I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code
Copy link

Thank you for the pull request, @ggetz!

✅ We can confirm we have a CLA on file for you.

@jjspace
Copy link
Contributor

jjspace commented May 24, 2024

@ggetz make-zip is failing for a missing .eslintignore file

Copy link
Contributor

@jjspace jjspace left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a handful of comments.

I'll also add after actually seeing it in action I still don't think I like this new "flat config". It just seems harder to actually grok and parse what's happening in different parts of the project. but that's just me, no avoiding it if we want to keep up to date.

.github/workflows/cla.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
eslint.config.js Show resolved Hide resolved
eslint.config.js Show resolved Hide resolved
eslint.config.js Show resolved Hide resolved
eslint.config.js Show resolved Hide resolved
@ggetz
Copy link
Contributor Author

ggetz commented May 28, 2024

@jjspace This should be ready for another look.

@jjspace
Copy link
Contributor

jjspace commented May 28, 2024

Apparently prettier didn't like that merge but I touched it up quick.

LGTM, thanks @ggetz!

@jjspace jjspace merged commit 58ea653 into main May 28, 2024
9 checks passed
@jjspace jjspace deleted the update-eslint branch May 28, 2024 17:54
@ggetz ggetz mentioned this pull request May 30, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants