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

🔧 Add import/no-duplicates lint rule #1181

Merged
merged 11 commits into from
May 23, 2024

Conversation

TheAfroOfDoom
Copy link
Contributor

@TheAfroOfDoom TheAfroOfDoom commented May 19, 2024

Summary

In #1180 I noticed that my duplicate import statements weren't being caught by the linter. There's (at least) two lint rules that exist that can detect/fix these easily, so this PR's primary purpose is to add one of those lint rules.

ESLint's built-in rule relating to duplicate imports no-duplicate-imports doesn't have auto-fix for some reason. So instead we went with the eslint-plugin-import plugin which has import/no-duplicates (which can auto-fix these errors).

sample rule error/auto-fix
image

Supplemental changes

I was also getting errors when I was running npm run lint due to our @typescript-eslint version not supporting our my local TS version (5.4).

G:\Coding\Spyglass [main]> npm run lint

> @spyglassmc/root@1.0.0 lint
> eslint --cache --max-warnings=0 packages/*/{src,test}/**/*.{cts,mts,ts}

=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <5.1.0

YOUR TYPESCRIPT VERSION: 5.4.5

Please only submit bug reports when using the officially supported version.

=============

So I updated @typescript-eslint to the latest major version v7.9 (since v7.0 was the minimum version with TS 5.4 support anyway).


Upgrading @typescript-eslint to v7.9 led to some Unsupported engine warnings whenever I'd run an npm command:

image

I had node v18.17.1 installed locally, but we didn't actually specify anywhere a minimum node version for the Spyglass repo as a whole.

So I went with @typescript-eslint's recommended versions (listed in that warning above): ^18.18.0 || >=20.0.0

  • ^18.18.0 is LTS
  • >=20.0.0 is any modern node version

These engines felt like a reasonable requirement. Upgrading my local node to v18 LTS (v18.20.2) resolved these engine discrepancy warnings with @typescript-eslint.


I think after updating @typescript-eslint to support my local Typescript version, the @typescript-eslint/prefer-readonly began to emit warnings it had not previously. So I included those auto-fixes in this PR as well: ef9d9c1 (#1181).


Unrelated, we forgot to run npm i after updating mcdoc-cli from 0.1.3 to 0.1.5. So I did that to keep our package-lock.json up-to-date in d3f9713 (#1181).

@TheAfroOfDoom TheAfroOfDoom changed the title Misc linting things afro WIP Misc linting things afro May 19, 2024
@TheAfroOfDoom TheAfroOfDoom changed the title WIP Misc linting things afro 🔧 Add import/no-duplicates lint rule May 19, 2024
@TheAfroOfDoom TheAfroOfDoom marked this pull request as ready for review May 19, 2024 16:14
@NeunEinser
Copy link
Contributor

Would it be possible to add a rule for unnecessary imports as well?

@TheAfroOfDoom
Copy link
Contributor Author

Would it be possible to add a rule for unnecessary imports as well?

I'm not sure what you mean by this specifically, but the plugin I added eslint-plugin-import has a bunch of other import lint rules. feel free to look through that and see if it has what you're looking for

@NeunEinser
Copy link
Contributor

NeunEinser commented May 19, 2024

I mean this situation.
image

Ideally, you wouldn't be allowed to commit this. It can happen quite easily when changing code and forgetting to delete these imports is quite easy.

I skimmed through the possible rules there and didn't see anything immediately.

@SPGoding
Copy link
Member

https://eslint.org/docs/latest/rules/no-unused-vars probably works for unused vars in import

Copy link
Member

@SPGoding SPGoding left a comment

Choose a reason for hiding this comment

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

The @spyglassmc/mcdoc-cli version really should just be 0.1.0-PLACEHOLDER in package.json like every other package in this repository since the version is set by the release script and should not be changed manually.

.eslintrc.cjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
- these scripts are ran directly from `package.json`; they're not bundled
- match requirements specified by `@typescript-eslint`
- `^18.18.0` is LTS, and `>=20.0.0` is modern versions
…ur Typescript version

- `v7.2.0` added support for our TS version (`5.4.5`): https://github.com/typescript-eslint/typescript-eslint/releases/tag/v7.2.0
- upgrading to latest release since its same major version anyway
- `eslint-plugin-import`
- `eslint-import-resolver-typescript`
- seems like these are only now appearing after the upgrade to `@typescript-eslint@v7.9.0`?
@TheAfroOfDoom TheAfroOfDoom force-pushed the misc-linting-things-afro branch from b67b944 to 54a0183 Compare May 20, 2024 15:38
@TheAfroOfDoom TheAfroOfDoom requested a review from SPGoding May 20, 2024 15:39
@MulverineX MulverineX merged commit 0c5e505 into SpyglassMC:main May 23, 2024
3 checks passed
@TheAfroOfDoom TheAfroOfDoom deleted the misc-linting-things-afro branch May 23, 2024 23:17
TheAfroOfDoom added a commit to TheAfroOfDoom/Spyglass that referenced this pull request May 24, 2024
TheAfroOfDoom added a commit to TheAfroOfDoom/Spyglass that referenced this pull request May 24, 2024
commit a8431ea
Author: SPGoding <i@spgoding.com>
Date:   Thu May 23 16:54:49 2024 -0500

    🐛 Fix computing relative URIs (SpyglassMC#1177)

commit 0c5e505
Author: Afro <tehafroofdoom@gmail.com>
Date:   Thu May 23 17:53:36 2024 -0400

    🔧 Add `import/no-duplicates` lint rule (SpyglassMC#1181)

commit 50c2185
Author: SPGoding <i@spgoding.com>
Date:   Wed May 22 21:26:34 2024 -0500

    🔥 Remove CommandTreeRegistry (SpyglassMC#1190)

commit 6439d5f
Author: NeunEinser <daniel30797@gmail.com>
Date:   Thu May 23 04:24:54 2024 +0200

    🚧 Remove attributed type in favor of attributes array on all types (SpyglassMC#1182)

commit d99eeb6
Author: SPGoding <i@spgoding.com>
Date:   Wed May 22 21:22:37 2024 -0500

    ⬆️ Upgrade dprint (SpyglassMC#1191)

commit 9a303cc
Author: Afro <tehafroofdoom@gmail.com>
Date:   Wed May 22 22:21:45 2024 -0400

    🔧 Add long timeout arg to unit test launch configuration (SpyglassMC#1189)

commit 2b74e1f
Author: Nico314159 <nicolino.will@gmail.com>
Date:   Wed May 22 13:04:25 2024 -0700

    Implement 1.20.5+ item slots (SpyglassMC#1179)

commit d6c395f
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue May 21 02:58:56 2024 -0500

    ⬆️ Bump rexml from 3.2.5 to 3.2.8 in /docs (SpyglassMC#1153)

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit a1f81c3
Author: NeunEinser <daniel30797@gmail.com>
Date:   Mon May 20 22:15:26 2024 +0200

    🐛 Fix message reporting breaking on empty string (SpyglassMC#1155)
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.

4 participants