fix: take context.cwd into account for glob options of several rules
#371
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/rules/no-extraneous-dependencies.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/rules/no-import-module-exports.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/rules/no-namespace.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (9)
✨ Finishing Touches
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 (
|
![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 3ca67f2 in 1 minute and 19 seconds. Click for details.
- Reviewed
39lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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. src/rules/no-extraneous-dependencies.ts:331
- Draft comment:
Consider adding an explicit type annotation for the destructured parameter 'cwd' (e.g., { cwd: string }) in the testConfig function to improve clarity and type-safety. - 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% The suggestion is reasonable since explicit types can improve code clarity and catch errors. However, TypeScript can likely infer the type correctly from context.cwd usage. The change is minor and not critical for correctness. The code will work fine either way since TypeScript's type inference is reliable here. The comment is technically correct and follows good TypeScript practices. Adding explicit types can help catch errors earlier and improve code readability. While explicit types are good practice, this case is not critical since TypeScript can infer the type and the code works correctly without it. The comment doesn't point out an actual problem that needs fixing. The comment should be removed as it suggests a minor stylistic improvement rather than fixing a real issue. The code works correctly as is.
2. src/rules/no-extraneous-dependencies.ts:339
- Draft comment:
Verify that switching from path.resolve(c) to path.join(cwd, c) yields the intended resolution behavior; ensure that 'cwd' is an absolute path to avoid mismatches in glob patterns. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the behavior of a code change, which violates the rule against asking for confirmation or verification of intention. It also suggests ensuring a condition (thatcwdis an absolute path), which is not allowed. Therefore, this comment should be removed.
3. src/rules/no-extraneous-dependencies.ts:407
- Draft comment:
When passing { cwd: context.cwd } into testConfig for depsOptions, ensure that context.cwd is always defined; consider adding a fallback (e.g., process.cwd()) if it's not set. - 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% ESLint's RuleContext always provides a cwd property - it's a core part of ESLint's API. The cwd is needed for ESLint to function at all, so it will never be undefined. The comment is suggesting defensive programming for a case that can't actually occur in practice. I could be wrong about ESLint's API guarantees. Maybe there are edge cases where context.cwd could be undefined. Even if there were edge cases, ESLint itself would fail before reaching this code if cwd was undefined, as it's needed for basic ESLint functionality. The comment should be deleted as it suggests handling an impossible case - context.cwd is guaranteed to be defined by ESLint's core functionality.
Workflow ID: wflow_6SLcdT4MeWM9SSAw
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: 1
🧹 Nitpick comments (1)
src/rules/no-extraneous-dependencies.ts (1)
340-340: The path resolution fix looks correct, but document the additional minimatch option.The change from
path.resolve(c)topath.join(cwd, c)correctly addresses the core issue by resolving paths relative to the provided cwd. However, the addition of{ windowsPathsNoEscape: true }should be documented as it changes the minimatch behavior.Consider adding a comment explaining why the
windowsPathsNoEscape: trueoption was added:minimatch(filename, path.join(cwd, c), { windowsPathsNoEscape: true }), + // windowsPathsNoEscape: true prevents escaping backslashes on Windows paths
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/rules/no-extraneous-dependencies.ts(3 hunks)
🔇 Additional comments (1)
src/rules/no-extraneous-dependencies.ts (1)
407-412: Function calls consistently updated with context.cwd.All calls to
testConfigare properly updated to pass the context's cwd, ensuring consistent behavior across different dependency types.
🦋 Changeset detectedLatest commit: 3d94167 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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. |
JounQin
force-pushed
the
fix-no-extraneous-dependencies-cwd
branch
from
67dea2b to
6456a7d
Compare
JounQin
changed the title
context.cwd into account for several glob options
Signed-off-by: JounQin <[email protected]>
JounQin
changed the title
context.cwd into account for several glob optionscontext.cwd into account for glob options of several rules
Signed-off-by: JounQin <[email protected]>
commit: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
2 participants
When passing a cwd to the linter instance, the matchers of no-extraneous-dependencies don't work and don't respect the root folder. Fixed by incorporating the cwd into the minimatch call.
Important
Fixes
no-extraneous-dependenciesrule to respectcwdby updatingtestConfigto includecwdinminimatchcall.no-extraneous-dependenciesrule to respectcwdwhen set.testConfigfunction to includecwdinminimatchcall.testConfiginno-extraneous-dependencies.tsto acceptcwdparameter and use it inminimatch.testConfigincreateRuleto passcontext.cwd.This description was created by
for 3ca67f2. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit