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

Upgrade ESLint #332

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Upgrade ESLint #332

merged 4 commits into from
Dec 12, 2024

Conversation

KristjanESPERANTO
Copy link
Collaborator

@KristjanESPERANTO KristjanESPERANTO commented Dec 8, 2024

  1. Upgrade ESLint to v9
  2. Switch from deprecated eslintrc config to flat config
  3. Apply rules to codebase
  4. The linter workflow didn't work after the ESLint upgrade, so I did two things to fix this:
    1. Move the linter test to a separate job that runs only once. It is not necessary to run it with every node version.
    2. Since the GitHub action complained about the missing lock file, I added it to the repository. According to the node documentation, it is recommended to commit the lock file anyway: "This file is intended to be committed into source repositories, and serves various purposes...".

Should I rather outsource the adjustments to the workflows to an extra PR?

@KristjanESPERANTO KristjanESPERANTO force-pushed the eslint branch 2 times, most recently from d6af6a7 to f6ff8b7 Compare December 8, 2024 21:04
Copy link

socket-security bot commented Dec 8, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@stylistic/[email protected] Transitive: environment, filesystem +38 26.9 MB eslint-stylistic-bot
npm/[email protected] Transitive: environment, eval, filesystem, shell, unsafe +84 10.7 MB eslintbot
npm/[email protected] None 0 176 kB sindresorhus

🚮 Removed packages: npm/@stylistic/[email protected], npm/[email protected]

View full report↗︎

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Member

@derhuerst derhuerst left a comment

Choose a reason for hiding this comment

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

This looks quite good! 🙌

Do you know how much work it would be to get rid of the remaining stylistic changes? I don't want you to put even more work into wrangling @stylistic/*, but maybe there are some low-hanging fruit?

@KristjanESPERANTO
Copy link
Collaborator Author

I think we have adapted the rules quite well to this project and your reasonable preferences.

But I understand the desire to define as few exceptions as possible.

I see three options:

  1. Leave it as it is for the time being.
  2. Instead of activating all @stylistic rules, we could only activate the recommended ones. With this we would have to define far fewer exceptions.
  3. Another option would be to use ‘@antfu/eslint-config’ instead of all other ESLint plugins. The package is from a/the @stylistic maintainer and comes with a huge number of opinionated rules. With this the ESLint configuration file would only consist of two lines (if we don't define exception). I'm currently considering converting some projects to this. I see two main advantages: A very simple ESLint configuration and a lot of rules (not only for JavaScript files). If one does not like the settings of several rules, this of course would also bloat the config.

I would be open to all three (or you see another another one). But I would prefer to complete this PR so we can check the upgrade to ESLint 9 🙂

Then I could create pull requests for option 2 and 3 with manageable effort to see what the changes would look like if we don't define exceptions.

@KristjanESPERANTO
Copy link
Collaborator Author

Do you know how much work it would be to get rid of the remaining stylistic changes?

Oh, I got you wrong! But now I got it. Yes, I think by disabling or adapting one or two rules, we can get rid of these stylistic changes. I will soon see into it.

Copy link
Member

@derhuerst derhuerst left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge, except that package-lock.json is still checked in (I'd like to keep it out).

@KristjanESPERANTO
Copy link
Collaborator Author

I think this is ready to merge, except that package-lock.json is still checked in (I'd like to keep it out).

Just removed it. I reset my branch for that and used this chance to fix typos in my commit messages 😅

Copy link
Member

@derhuerst derhuerst left a comment

Choose a reason for hiding this comment

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

Thanks! I have slightly renamed your commits.

@derhuerst derhuerst merged commit 3fe9fc6 into public-transport:main Dec 12, 2024
9 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants