docs: add why section on the top to clarify
#323
Conversation
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
WalkthroughThe changes update the staged linting tool from Changes
Assessment against linked issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
WalkthroughThis pull request introduces a new Changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #323 +/- ##
=======================================
Coverage 96.02% 96.02%
=======================================
Files 92 92
Lines 4758 4758
Branches 1768 1789 +21
=======================================
Hits 4569 4569
Misses 188 188
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
commit: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 32a6b98 in 2 minutes and 9 seconds. Click for details.
- Reviewed
92lines of code in3files - Skipped
1files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .nano-staged.js:1
- Draft comment:
Correctly updated export from '@1stg/lint-staged/tsc' to '@1stg/nano-staged/tsc'. Ensure all tooling and docs refer to the new package. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. README.md:56
- Draft comment:
Consider revising some phrasing in the new 'Why' section (e.g., use 'cannot' instead of 'can not' and 'what are the differences' instead of 'what's the differences') for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar suggestions are technically correct, they are very minor style changes that don't impact understanding. The text is still perfectly clear as written. PR comments should focus on more substantive issues. Additionally, this is documentation text, not code logic. The grammar issues could make the documentation look less professional. Some readers may notice these small errors. The benefit of fixing these minor grammar issues is outweighed by the noise of having a PR comment about them. The text is understandable as-is. Delete this comment as it focuses on minor style issues rather than substantive changes that would meaningfully improve the code or documentation.
3. package.json:87
- Draft comment:
Updated devDependency from '@1stg/lint-staged' to '@1stg/nano-staged'. Confirm that all related scripts and hooks are aligned with this change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. README.md:17
- Draft comment:
Typographical note: Consider using the consistent and official casing "TypeScript" instead of "Typescript" in the phrase "dependency on Typescript". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the official casing being "TypeScript", this is a very minor typographical issue in documentation. The meaning is completely clear either way. The rules say not to make comments that are obvious or unimportant. Casing in documentation is not a critical issue that requires a code change. The official branding of TypeScript could be considered important for consistency and professionalism in documentation. Some projects do care about getting branding exactly right. While branding consistency is nice to have, this is too minor of an issue to warrant a PR comment. It doesn't affect functionality and the meaning is clear either way. Delete this comment as it points out a very minor typographical issue that doesn't meaningfully impact the documentation's clarity or accuracy.
5. README.md:58
- Draft comment:
Typo: Consider changing "can not" to "cannot" for a more standard usage. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While this is technically a valid grammar suggestion, it's an extremely minor stylistic change that doesn't impact code quality or functionality. Both "can not" and "cannot" are grammatically acceptable, though "cannot" is more common. The rules state not to make comments that are obvious or unimportant, and this seems to fall into that category. The change would make the text slightly more polished and professional. Some style guides do explicitly recommend "cannot" over "can not". While technically correct, this kind of minor prose style suggestion in documentation doesn't warrant a PR comment. It doesn't meaningfully improve the codebase. This comment should be deleted as it suggests an unimportant stylistic change that doesn't meaningfully improve the documentation.
6. README.md:71
- Draft comment:
Grammar issue: "So what's the differences" should be rephrased to "So what are the differences" or "So what's different". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is about a real grammatical error in the text. However, the rules state to not make purely informative comments and to only comment if there is clearly a code change required. While this is a valid grammar correction, it's a minor documentation issue that doesn't affect functionality. The rules emphasize focusing on logic issues rather than pure text/documentation changes. The grammar error could make the documentation less professional and harder to understand for non-native English speakers. Documentation quality is important for open source projects. While documentation quality matters, this is a minor grammatical issue that doesn't significantly impact understanding. The rules explicitly state to focus on code logic issues and not make purely informative comments. The comment should be deleted as it violates the rule about not making purely informative comments. The grammatical error is minor and doesn't warrant a PR comment.
Workflow ID: wflow_25EMChWJpcftjLPe
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
17-18: Clarify project introduction phrasingThe sentence “It starts as a fork of [
eslint-plugin-import] using [get-tsconfig] to replace …” is informal and passive. Consider rephrasing for clarity and directness, for example:- It starts as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] … + This project forks [`eslint-plugin-import`] and replaces [`tsconfig-paths`] and heavy [`typescript`] with [`get-tsconfig`] to streamline dependencies.
56-63: Improve wording and grammar in Why section
- Replace “a lot of issues can not be fixed easily without API changes” with a more concise alternative, e.g.:
- Because a lot of issues can not be fixed easily without API changes, see + Many issues cannot be resolved without API changes; see- Use “cannot” instead of “can not” for consistency.
🧰 Tools
🪛 LanguageTool
[style] ~58-~58: Consider using a synonym to be more concise.
Context: ... - License ## Why Because a lot of issues can not be fixed easily without ...(A_LOT_OF)
[style] ~58-~58: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cense) ## Why Because a lot of issues can not be fixed easily without API changes, se...(CAN_NOT_PREMIUM)
[style] ~58-~58: Consider using a different verb for a more formal wording.
Context: ...Why Because a lot of issues can not be fixed easily without API changes, see - <htt...(FIX_RESOLVE)
64-69: Use consistent casing and concise phrasing for “breaking changes”
- Avoid ALL CAPS for “BREAKING CHANGES”—consider “Breaking changes” instead.
- Rephrase for brevity:
- And then it claimed to be allowed to update with BREAKING CHANGES at …, but it still doesn't happen or even seems to happen. + Although support for Breaking changes was promised in issue #170, it still hasn’t materialized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
.nano-staged.js(1 hunks)README.md(3 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~58-~58: Consider using a synonym to be more concise.
Context: ... - License ## Why Because a lot of issues can not be fixed easily without ...
(A_LOT_OF)
[style] ~58-~58: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cense) ## Why Because a lot of issues can not be fixed easily without API changes, se...
(CAN_NOT_PREMIUM)
[style] ~58-~58: Consider using a different verb for a more formal wording.
Context: ...Why Because a lot of issues can not be fixed easily without API changes, see - <htt...
(FIX_RESOLVE)
[style] ~77-~77: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...ingle resolver instance which is used all across resolving chains so it would benefit fr...
(EN_WORDINESS_PREMIUM_ALL_ACROSS)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: autofix
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
🔇 Additional comments (3)
README.md (1)
25-25: TOC updated with Why sectionThe new “Why” entry in the Table of Contents correctly reflects the added section. Nice catch updating the TOC.
package.json (1)
88-88:✅ Verification successful
Verify complete replacement of lint-staged tooling
Ensure
@1stg/lint-stagedhas been removed from all references (scripts, configs) and that nothing still points to it. You can run:
🏁 Script executed:
#!/bin/bash # Verify removal of lint-staged references rg --ignore-case '@1stg/lint-staged' || echo "No @1stg/lint-staged references found."Length of output: 123
@1stg/lint-staged references fully removed
A global search (
rg --ignore-case '@1stg/lint-staged') returned no matches, confirming that all references to the old lint-staged tooling have been removed..nano-staged.js (1)
1-1: Correctly re-exporting nano-stagedThe file cleanly re-exports the default from
@1stg/nano-staged/tsc, aligning with the updated devDependency. Looks good.
|
Caution No docstrings were generated. |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 35a0087 in 1 minute and 59 seconds. Click for details.
- Reviewed
96lines of code in3files - Skipped
1files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .nano-staged.js:1
- Draft comment:
Verify the update from '@1stg/lint-staged/tsc' to '@1stg/nano-staged/tsc' is intentional and the new package is correctly integrated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. README.md:57
- Draft comment:
The new 'Why' and 'Differences' sections add valuable context. Consider minor rephrasing for clarity (e.g. 'cannot' instead of 'can not' and 'What are the differences' instead of 'So what's the differences'). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. package.json:85
- Draft comment:
Updated devDependency from '@1stg/lint-staged' to '@1stg/nano-staged'. Please confirm the new version (^0.1.1) is compatible with the project requirements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. README.md:17
- Draft comment:
There is a capitalization inconsistency with 'typescript'. The code snippet usestypescriptin lowercase, but later in the sentence it appears as 'Typescript'. Consider using 'TypeScript' consistently throughout the file to match the official branding. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment is technically correct that TypeScript's official branding uses that capitalization, this seems like a minor stylistic issue. The meaning is clear regardless of capitalization. The comment doesn't point out a real code issue or bug. It's more of a nitpick about documentation style. The official TypeScript branding could be considered important for a professional open source project. Consistency in documentation can reflect attention to detail. While branding consistency is nice to have, this kind of minor documentation nitpick creates noise and doesn't meaningfully improve the code quality or functionality. Delete this comment as it's too minor and doesn't identify a significant issue that needs fixing.
5. README.md:59
- Draft comment:
Typographical suggestion: Consider replacing "can not" with "cannot" for a more standard form. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The rules state to NOT make comments that are obvious or unimportant. This is a very minor stylistic suggestion about documentation that doesn't impact functionality. While technically correct that "cannot" is more standard, this level of nitpicking about documentation wording is not valuable enough to warrant a comment. The suggestion is technically correct from a formal writing perspective. Documentation quality and consistency does matter for open source projects. While documentation quality matters, this extremely minor stylistic suggestion doesn't rise to the level of importance that would justify asking the author to make a change. Both forms are widely understood. Delete this comment as it is too minor and unimportant to warrant asking for a change.
6. README.md:74
- Draft comment:
Typographical note: The phrase "what's the differences" sounds off. You might want to change it to "what are the differences" or "what's the difference" based on your intended meaning. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment points out a real grammatical issue, it's a very minor one that doesn't affect code functionality or documentation clarity. The rules state not to make purely informative comments or comments that are obvious/unimportant. Grammar nitpicks generally fall into this category unless they significantly impact understanding. The grammar error could potentially make the documentation look less professional. Some might argue that maintaining high quality even in small details is important for documentation. While professionalism is important, this is a very minor grammatical issue that doesn't impede understanding. The rules explicitly say not to make unimportant comments or purely informative ones. This comment should be deleted as it's too minor and purely informative, violating the rules about not making unimportant comments.
7. README.md:80
- Draft comment:
Minor phrasing note: Consider changing "memoization out of box" to "memoization out of the box" to improve clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the suggestion is technically correct English, it's a very minor documentation wording issue. The meaning is still clear with the current phrasing. The rules state not to make purely informative comments or comments that are obvious/unimportant. Documentation phrasing suggestions like this don't require code changes. The phrase "out of box" could be considered slightly confusing to non-native English speakers. Clear documentation is important. While clear docs are good, this is an extremely minor phrasing issue that doesn't impact understanding. The current phrase is still understandable. Delete this comment as it's a minor documentation phrasing suggestion that doesn't require code changes.
Workflow ID: wflow_lJEc9SKbldlBGxxX
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
README.md (6)
17-17: Refine fork rationale for conciseness and clarityCurrent sentence is lengthy and repeats “heavy” twice. Consider rewriting for a more concise narrative. For example:
- It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster, through less heavy dependency on Typescript, and cleaner dependencies altogether. + It began as a fork of [`eslint-plugin-import`], replacing `tsconfig-paths` and `typescript` with the lightweight [`get-tsconfig`] for faster performance and a cleaner dependency tree.
57-64: Improve "Why" section wordingThe introduction uses informal phrasing and “can not”. Consider a more formal rewrite:
- Because a lot of issues can not be fixed easily without API changes, see + Many issues cannot be resolved without breaking API changes; see: - https://github.com/import-js/eslint-plugin-import/issues/1479 - https://github.com/import-js/eslint-plugin-import/issues/2108 - https://github.com/import-js/eslint-plugin-import/issues/2111🧰 Tools
🪛 LanguageTool
[style] ~59-~59: Consider using a synonym to be more concise.
Context: ... - License ## Why Because a lot of issues can not be fixed easily without ...(A_LOT_OF)
[style] ~59-~59: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cense) ## Why Because a lot of issues can not be fixed easily without API changes, se...(CAN_NOT_PREMIUM)
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...Why Because a lot of issues can not be fixed easily without API changes, see - <htt...(FIX_RESOLVE)
65-68: Clarify rationale for forking due to API change refusalThe phrasing is a bit informal. For example:
- But [`eslint-plugin-import`] refused to accept BREAKING CHANGES for these issues, so we have to fork it. + However, [`eslint-plugin-import`] declined to accept these breaking API changes, necessitating our fork.And for the subsequent sentence:
- And then [`eslint-plugin-import`] claimed to be allowed to update with BREAKING CHANGES at <link>, but it still doesn't happen or even seems to happen: <link>. + Although [`eslint-plugin-import`] later indicated openness to breaking changes (see <link>), no such updates have materialized.
70-70: Clarify future work statementRewrite for clarity and active voice:
- We haven't resolved all the issues yet, but we are working on it which could happen in the next major version (v5): <link>. + While not all issues are resolved, we plan to address them in the next major version (v5): <link>.🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: Possible missing comma found.
Context: ...l the issues yet, but we are working on it which could happen in the next major ve...(AI_HYDRA_LEO_MISSING_COMMA)
76-82: Standardize bullet capitalization in Differences sectionBullets currently start with lowercase; for consistency, capitalize the first word of each item:
- - we target [Node …] + - We target [Node …] - - we don't depend on old and outdated dependencies… + - We don't depend on old and outdated dependencies… - …This aligns style with other sections and improves readability.
🧰 Tools
🪛 LanguageTool
[style] ~80-~80: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...(EN_WORDINESS_PREMIUM_ALL_ACROSS)
697-704: Organize and alphabetize new reference definitionsThe new footnote definitions (
[get-tsconfig], [tsconfig-paths], [typescript], [unrs-resolver],[napi-rs]) are appended but out of order. Consider grouping all custom references together and sorting them (e.g., alphabetically or by first use) to simplify maintenance.Also applies to: 710-710
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
.nano-staged.js(1 hunks)README.md(3 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- .nano-staged.js
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~59-~59: Consider using a synonym to be more concise.
Context: ... - License ## Why Because a lot of issues can not be fixed easily without ...
(A_LOT_OF)
[style] ~59-~59: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cense) ## Why Because a lot of issues can not be fixed easily without API changes, se...
(CAN_NOT_PREMIUM)
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...Why Because a lot of issues can not be fixed easily without API changes, see - <htt...
(FIX_RESOLVE)
[uncategorized] ~70-~70: Possible missing comma found.
Context: ...l the issues yet, but we are working on it which could happen in the next major ve...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~80-~80: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...
(EN_WORDINESS_PREMIUM_ALL_ACROSS)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
🔇 Additional comments (1)
README.md (1)
25-26: TOC updated with new sectionsThe manual Table of Contents now includes links to the new “Why” and “Differences” sections, matching the added headings below.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0d2833a in 1 minute and 53 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. README.md:17
- Draft comment:
Consider rephrasing for clarity and consistency. For example, you could write: "It started as a fork of [eslint-plugin-import] that uses [get-tsconfig] to replace [tsconfig-paths] and reduce its heavy dependency on TypeScript, thereby improving speed and streamlining dependencies." This also ensures consistent capitalization of 'TypeScript'. - Reason this comment was not posted:
Comment was on unchanged code.
2. README.md:17
- Draft comment:
It looks like "typescript" is referenced in two different ways in this line – once astypescript(in code style) and once in the link text as "Typescript". Consider using the official and consistent spelling "TypeScript" in both places. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_lDSE4QJ3iBYUApK1
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
17-18: Refine introductory sentence for clarity and concisenessThe new sentence is quite long and a bit wordy. Consider a more direct phrasing to improve readability.
-It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster, through less [heavy dependency on Typescript](https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/src/exportMap/typescript.js#L16), and cleaner dependencies altogether. +It began as a fork of [`eslint-plugin-import`] to replace `tsconfig-paths` and `typescript` with `get-tsconfig`, reducing dependency weight and improving performance.
59-64: Improve grammar and style in the introductory paragraphTo sound more formal and concise, replace “a lot of” with “many” and use “cannot” as one word. Also, rephrase for clarity:
-Because a lot of issues can not be fixed easily without API changes, see +Many issues cannot be resolved without API changes. See:🧰 Tools
🪛 LanguageTool
[style] ~59-~59: Consider using a synonym to be more concise.
Context: ... - License ## Why Because a lot of issues can not be fixed easily without ...(A_LOT_OF)
[style] ~59-~59: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cense) ## Why Because a lot of issues can not be fixed easily without API changes, se...(CAN_NOT_PREMIUM)
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...Why Because a lot of issues can not be fixed easily without API changes, see - <htt...(FIX_RESOLVE)
65-71: Enhance flow by removing leading conjunctionsAvoid starting sentences with “But” or “And then” for a cleaner narrative. For example:
-But [`eslint-plugin-import`] refused to accept BREAKING CHANGES for these issues, so we have to fork it. +However, [`eslint-plugin-import`] declined these breaking changes, necessitating our fork. -And then [`eslint-plugin-import`] claimed to be allowed to update with BREAKING CHANGES at <https://github.com/un-ts/eslint-plugin-import-x/issues/170>, but it still doesn't happen or even seems to happen: <https://github.com/import-js/eslint-plugin-import/pull/3091>. +Later, [`eslint-plugin-import`] indicated a willingness to introduce breaking changes (see https://github.com/un-ts/eslint-plugin-import-x/issues/170), but no progress has been made (https://github.com/import-js/eslint-plugin-import/pull/3091).This also tightens the language and improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~59-~59: Consider using a synonym to be more concise.
Context: ... - License ## Why Because a lot of issues can not be fixed easily without ...
(A_LOT_OF)
[style] ~59-~59: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...cense) ## Why Because a lot of issues can not be fixed easily without API changes, se...
(CAN_NOT_PREMIUM)
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...Why Because a lot of issues can not be fixed easily without API changes, see - <htt...
(FIX_RESOLVE)
[style] ~80-~80: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...
(EN_WORDINESS_PREMIUM_ALL_ACROSS)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
🔇 Additional comments (3)
README.md (3)
25-26: TOC updated to include new sectionsThe Table of Contents now correctly lists the newly added "Why" and "Differences" sections.
57-58: New “Why” section heading inserted appropriatelyThe heading introduces the rationale at the top of the README and follows the existing style.
697-697: Link references for added dependencies are present and correctThe reference URLs for
eslint-plugin-import,get-tsconfig,tsconfig-paths,typescript, andunrs-resolvermatch the labels introduced above. No issues found.Also applies to: 701-704
Co-authored-by: Ben McCann <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
README.md (5)
17-17: Consider improving clarity and grammar in the fork rationale
This sentence is lengthy and can be split for readability; also correct “heavy dependency on Typescript” to “heavy TypeScript dependency” and simplify phrasing.Example diff:
-It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster, through less [heavy dependency on Typescript](https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/src/exportMap/typescript.js#L16), and cleaner dependencies altogether. +This project began as a fork of [`eslint-plugin-import`], replacing [`tsconfig-paths`] and the heavy [`typescript`]-based loader with [`get-tsconfig`] to reduce dependency bloat and improve performance.
59-63: Revise wording for readability and consistency
Use “cannot” instead of “can not”, replace “A lot of issues” with “Many issues”, and refine the lead-in to the bullet list. Also consider adding descriptive text for each issue link.Example:
-A lot of issues can not be fixed easily without API changes. E.g. see: +Many issues cannot be resolved without breaking API changes. For example:🧰 Tools
🪛 LanguageTool
[style] ~59-~59: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...nse](#license) ## Why A lot of issues can not be fixed easily without API changes. E....(CAN_NOT_PREMIUM)
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...se) ## Why A lot of issues can not be fixed easily without API changes. E.g. see: ...(FIX_RESOLVE)
65-65: Wrap “breaking changes” in code ticks and lowercase for consistency
Standardize styling: use backticks forbreaking changesand prefer lowercase.Example:
-[`eslint-plugin-import`] refused to accept BREAKING CHANGES for these issues, so we had to fork it. +[`eslint-plugin-import`] refused to accept `breaking changes` for these issues, so we had to fork it.
67-67: Soften tone and clarify progress statement
Replace “claims” with “states” and “still nothing is happening” with “no progress has been made” for neutrality.Example:
-[`eslint-plugin-import`] now claims in <https://github.com/un-ts/eslint-plugin-import-x/issues/170> that it will accept BREAKING CHANGES. However, still nothing is happening: <https://github.com/import-js/eslint-plugin-import/pull/3091>. +[`eslint-plugin-import`] now states in <https://github.com/un-ts/eslint-plugin-import-x/issues/170> that it will accept breaking changes, but no progress has been made: <https://github.com/import-js/eslint-plugin-import/pull/3091>.
69-69: Clarify the timeline and add comma for grammar
Add a comma before “which” and make it clear what will happen in the next major version.Example:
-We haven't resolved all the issues yet, but we are working on it which could happen in the next major version (v5): <https://github.com/un-ts/eslint-plugin-import-x/issues/235>. +We haven’t resolved all issues yet, but we are actively working on them, which we expect to address in the next major version (v5): <https://github.com/un-ts/eslint-plugin-import-x/issues/235>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~58-~58: Consider using a synonym to be more concise.
Context: ...angelog) - License ## Why A lot of issues can not be fixed easily without ...
(A_LOT_OF)
[style] ~59-~59: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...nse](#license) ## Why A lot of issues can not be fixed easily without API changes. E....
(CAN_NOT_PREMIUM)
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...se) ## Why A lot of issues can not be fixed easily without API changes. E.g. see: ...
(FIX_RESOLVE)
[style] ~79-~79: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...
(EN_WORDINESS_PREMIUM_ALL_ACROSS)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
🔇 Additional comments (2)
README.md (2)
25-26: TOC entries for new sections look correct
The added- [Why](#why)and- [Differences](#differences)align with the new headings.
57-57: New “Why” section introduced appropriately
The heading clearly marks the rationale section at the top.
|
@benmccann Great thanks! Just let me know when all suggestions are made so that I can apply them once together. |
Co-authored-by: Ben McCann <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (2)
17-18: Refine the fork rationale sentence for clarity
The current sentence is lengthy and mixes multiple points. Consider splitting it and reordering to improve readability.
Proposed diff:-It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster, through less [heavy dependency on Typescript], and cleaner dependencies altogether. +This plugin began as a fork of [`eslint-plugin-import`] to reduce dependency weight and improve speed. It replaces `tsconfig-paths` and the heavy [`typescript`] dependency with [`get-tsconfig`], resulting in a leaner and faster implementation.
71-83: Improve bullet consistency in “Differences” section
- Capitalize the first word of each bullet for parallel structure.
- Consider rephrasing the question at line 73 to “What are the exact differences from
eslint-plugin-import?”
Example:## Differences What are the exact differences from `eslint-plugin-import`? - We target Node `^18.18.0 || ^20.9.0 || >=21.1.0` and ESLint `^8.57.0 || ^9.0.0`, while `eslint-plugin-import` targets Node `>=4` and ESLint `^2 || ... || ^9`. - We have only 49 dependencies versus 117 in `eslint-plugin-import`. - We load tsconfig files with the lightweight `get-tsconfig` package instead of `tsconfig-paths` + `typescript`. - We use `unrs-resolver` (a `napi-rs` project) instead of `resolve` for faster support of the `exports` field. - Our v3 resolver shares a single instance for built-in caching and memoization. - ...🧰 Tools
🪛 LanguageTool
[style] ~79-~79: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...(EN_WORDINESS_PREMIUM_ALL_ACROSS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~58-~58: Consider using a synonym to be more concise.
Context: ...angelog) - License ## Why A lot of issues can not be fixed easily without ...
(A_LOT_OF)
[style] ~59-~59: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...nse](#license) ## Why A lot of issues can not be fixed easily without API changes. E....
(CAN_NOT_PREMIUM)
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...se) ## Why A lot of issues can not be fixed easily without API changes. E.g. see: ...
(FIX_RESOLVE)
[uncategorized] ~69-~69: Possible missing comma found.
Context: ...l the issues yet, but we are working on it which could happen in the next major ve...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~79-~79: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...
(EN_WORDINESS_PREMIUM_ALL_ACROSS)
🪛 markdownlint-cli2 (0.17.2)
README.md
709-709: Link and image reference definitions should be needed
Unused link or image reference definition: "napi-rs"
(MD053, link-image-reference-definitions)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
🔇 Additional comments (2)
README.md (2)
25-26: Include new sections in the Table of Contents
The new "Why" and "Differences" sections are correctly added to the TOC.
696-703: Link reference definitions appear correct
All new link definitions are used in the document. No unused references detected.Also applies to: 709-709
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 360a34b in 44 seconds. Click for details.
- Reviewed
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. README.md:57
- Draft comment:
Improved phrasing in the 'Why' section for clarity: replacing 'A lot of issues can not' with 'Many issues cannot' and 'E.g. see:' with 'For example, see:'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. README.md:67
- Draft comment:
Good update for subject-verb agreement: 'working on it' changed to 'working on them' to match the plural 'issues'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. README.md:698
- Draft comment:
Added the [napi-rs] reference link for clarity in the Differences section; ensure it is correctly linked. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. README.md:706
- Draft comment:
Removed duplicate [napi-rs] reference link to avoid redundancy in the dependency links section. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_CQoLQ8gFYP32H8O7
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
README.md (6)
17-17: Clarify sentence structure and standardize casing
The new sentence begins with "It started…", which can be ambiguous. Consider rephrasing to start with the subject ("This plugin") and standardizing "TypeScript". Also simplify the dependency clause for readability.-It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster, through less [heavy dependency on Typescript](https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/src/exportMap/typescript.js#L16), and cleaner dependencies altogether. +This plugin started as a fork of [`eslint-plugin-import`], replacing [`tsconfig-paths`] and the heavy [`typescript`] dependency with [`get-tsconfig`] under the hood for better performance and cleaner dependencies.
57-64: Refine the “Why” introduction
- Emphasize that the unresolved issues require breaking API changes.
- Use “resolved” instead of “fixed” for a formal tone.
-## Why - -Many issues cannot be fixed easily without API changes. For example, see: +## Why + +Many issues cannot be resolved without breaking API changes. For example:🧰 Tools
🪛 LanguageTool
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...license) ## Why Many issues cannot be fixed easily without API changes. For example...(FIX_RESOLVE)
65-68: Streamline the fork rationale
Combine and polish the transition to improve flow and reduce wordiness.-[`eslint-plugin-import`] refused to accept BREAKING CHANGES for these issues, so we had to fork it. +[`eslint-plugin-import`] refused to accept the required breaking changes; therefore, we forked the project. -[`eslint-plugin-import`] now claims in <https://github.com/un-ts/eslint-plugin-import-x/issues/170> that it will accept BREAKING CHANGES. However, still nothing is happening: <https://github.com/import-js/eslint-plugin-import/pull/3091>. +Although [`eslint-plugin-import`] now claims it will accept breaking changes (<https://github.com/un-ts/eslint-plugin-import-x/issues/170>), no progress has been made: <https://github.com/import-js/eslint-plugin-import/pull/3091>.
69-70: Improve clarity of the closing statement
Split into two sentences and tighten phrasing for readability.-We haven't resolved all the issues yet, but we are working on them, which could happen in the next major version (v5): <https://github.com/un-ts/eslint-plugin-import-x/issues/235>. +We haven’t resolved all issues yet, but we’re actively working on them. Changes may land in the next major version (v5): <https://github.com/un-ts/eslint-plugin-import-x/issues/235>.
75-80: Standardize bullet style in “Differences”
The list mixes lowercase starts, uppercase starts, and inconsistent punctuation. For consistency, begin each bullet with an uppercase letter and end with a period. Optionally, you can rewrite for parallel structure:-- we target Node `^18.18.0 || ^20.9.0 || >=21.1.0` + ESLint `^8.57.0 || ^9.0.0`, while `eslint-plugin-import` targets Node `>=4` and ESLint `^2 || ^3 || …`. +- We target Node `^18.18.0 || ^20.9.0 || >=21.1.0` and ESLint `^8.57.0 || ^9.0.0`, whereas `eslint-plugin-import` supports Node `>=4` and ESLint `^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0 || ^8 || ^9`.(Apply similar normalization to the remaining bullets.)
🧰 Tools
🪛 LanguageTool
[style] ~79-~79: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...(EN_WORDINESS_PREMIUM_ALL_ACROSS)
79-79: Replace “all across” with a concise alternative
Avoid wordiness by using “throughout” instead of “all across”:-Our [v3 resolver] interface shares a single `resolver` instance by default which is used all across resolving chains so it would benefit from caching and memoization out-of-the-box. +Our [v3 resolver] interface shares a single `resolver` instance by default throughout resolving chains, benefiting from caching and memoization out of the box.🧰 Tools
🪛 LanguageTool
[style] ~79-~79: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...(EN_WORDINESS_PREMIUM_ALL_ACROSS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...license) ## Why Many issues cannot be fixed easily without API changes. For example...
(FIX_RESOLVE)
[style] ~79-~79: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...
(EN_WORDINESS_PREMIUM_ALL_ACROSS)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
🔇 Additional comments (2)
README.md (2)
25-26: Verify Table of Contents entries
The newly added entries for "Why" and "Differences" correctly reference#whyand#differencesanchors and match the added sections below.
696-696: Confirm reference definitions for new labels
All new link references (eslint-plugin-import,get-tsconfig,napi-rs,tsconfig-paths,typescript,unrs-resolver) are properly defined at the end of the file.Also applies to: 700-704
Co-authored-by: Sukka <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
17-18: Use correct casing and refine phrasing for clarity
The sentence mixes casing (“Typescript”) and the phrase “through less heavy dependency” is awkward. Consider rephrasing and using the proper product nameTypeScript.-It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster, through less [heavy dependency on Typescript](https://github.com/import-js/eslint-plugin-import/blob/da5f6ec13160cb288338db0c2a00c34b2d932f0d/src/exportMap/typescript.js#L16), and cleaner dependencies altogether. +It started as a fork of [`eslint-plugin-import`] using [`get-tsconfig`] to replace [`tsconfig-paths`] and heavy [`typescript`] under the hood, making it faster with a lighter dependency on [TypeScript], and resulting in a cleaner dependency graph.
57-71: Polish the “Why” section for style and consistency
A few minor tweaks can improve readability and tone:
- Replace “still nothing is happening” with “no progress has been made.”
- Use “Meanwhile” instead of “In the meantime” for conciseness.
- Normalize casing/formatting of “Breaking Changes” (e.g. sentence-case or title-case consistently).
## Why -Many issues cannot be fixed easily without API changes. For example, see: +Many issues cannot be fixed easily without breaking API changes. For example: - [`eslint-plugin-import`] refused to accept BREAKING CHANGES for these issues, so we had to fork it. +[`eslint-plugin-import`] refused to accept breaking changes for these issues, so we forked it. -[`eslint-plugin-import`] now claims in <https://github.com/un-ts/eslint-plugin-import-x/issues/170> that it will accept BREAKING CHANGES. However, still nothing is happening: <https://github.com/import-js/eslint-plugin-import/pull/3091>. +[`eslint-plugin-import`] now claims in <https://github.com/un-ts/eslint-plugin-import-x/issues/170> that it will accept breaking changes. However, no progress has been made: <https://github.com/import-js/eslint-plugin-import/pull/3091>. -[`eslint-plugin-import`] refuses to support the `exports` feature, and the maintainer even locked the feature request issue <https://github.com/import-js/eslint-plugin-import/issues/1810> to prevent future discussion. In the meantime, `eslint-plugin-import-x` now provides first-party support for the `exports` feature <https://github.com/un-ts/eslint-plugin-import-x/pull/209>, which will become the default in the next major version (v5). +[`eslint-plugin-import`] refuses to support the `exports` feature and even locked the feature request issue <https://github.com/import-js/eslint-plugin-import/issues/1810>. Meanwhile, `eslint-plugin-import-x` provides first-party support for `exports` <https://github.com/un-ts/eslint-plugin-import-x/pull/209>, which will become the default in v5. -We haven't resolved all the issues yet, but we are working on them, which could happen in the next major version (v5): <https://github.com/un-ts/eslint-plugin-import-x/issues/235>. +We haven’t resolved all issues yet, but we’re working on them and expect to address them in the next major version (v5): <https://github.com/un-ts/eslint-plugin-import-x/issues/235>.🧰 Tools
🪛 LanguageTool
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...license) ## Why Many issues cannot be fixed easily without API changes. For example...(FIX_RESOLVE)
[style] ~69-~69: ‘In the meantime’ might be wordy. Consider a shorter alternative.
Context: ...ues/1810> to prevent future discussion. In the meantime,eslint-plugin-import-xnow provides ...(EN_WORDINESS_PREMIUM_IN_THE_MEANTIME)
702-706: Remove unused link reference
The[napi-rs]reference is defined but never used in the document. You can remove this line to satisfy markdownlint and keep the link definitions tidy.-[`napi-rs`]: https://github.com/napi-rs/napi-rs🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
703-703: Link and image reference definitions should be needed
Unused link or image reference definition: "napi-rs"(MD053, link-image-reference-definitions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~59-~59: Consider using a different verb for a more formal wording.
Context: ...license) ## Why Many issues cannot be fixed easily without API changes. For example...
(FIX_RESOLVE)
[style] ~69-~69: ‘In the meantime’ might be wordy. Consider a shorter alternative.
Context: ...ues/1810> to prevent future discussion. In the meantime, eslint-plugin-import-x now provides ...
(EN_WORDINESS_PREMIUM_IN_THE_MEANTIME)
[style] ~81-~81: ‘all across’ might be wordy. Consider a shorter alternative.
Context: ...lver` instance by default which is used all across resolving chains so it would benefit fr...
(EN_WORDINESS_PREMIUM_ALL_ACROSS)
[uncategorized] ~81-~81: A comma might be missing here.
Context: ...ault which is used all across resolving chains so it would benefit from caching and me...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 markdownlint-cli2 (0.17.2)
README.md
703-703: Link and image reference definitions should be needed
Unused link or image reference definition: "napi-rs"
(MD053, link-image-reference-definitions)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
🔇 Additional comments (1)
README.md (1)
25-26: Add “Why” and “Differences” to the Table of Contents
The new TOC entries correctly link to the newly introduced sections and improve navigation.
|
Maybe @benmccann want to take another look again since @SukkaW reviews and suggestions. |
close #196
cc @43081j, @adrian-gierakowski , @benmccann and @JoshuaKGoldberg
I'm not a native English speaker, so maybe you can help to improve the sentences. ❤️
also cc @SukkaW
Important
Added a "Why" section to the README and updated a development dependency for improved tooling.
README.mdexplaining the rationale for forking and key differences from the original plugin.README.mdto include new sections.@1stg/lint-stagedwith@1stg/nano-stagedinpackage.jsonand.nano-staged.jsfor improved tooling.This description was created by
for 360a34b. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Documentation
Chores