fix: requiring . or ./ should respect mainFiles option#163
Conversation
WalkthroughThe updates streamline the resolution logic for Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Resolver
participant Loader
Caller->>Resolver: require_relative(specifier, ...)
alt specifier is "."
Resolver->>Loader: load_as_file_or_directory(cached_path, "./")
else
Resolver->>Loader: load_as_file_or_directory(cached_path, specifier)
end
Loader-->>Resolver: Resolution result or NotFound
Resolver-->>Caller: Return result or error
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
✨ 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how . and ./ are resolved so that they respect the mainFiles option and adds tests to verify behavior when mainFiles is empty. It also updates an existing test to simplify its dependency assertions.
- Replaces manual special-casing for
././inResolverGenericwith a unifiedload_as_file_or_directorycall. - Adds tests in
resolve.rsto assertNotFounderrors whenmainFilesis set to an empty list. - Simplifies
incorrect_description_file_1test to only assert the expected package file dependency.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/resolve.rs | Updated resolve_dot tests and added cases for empty mainFiles |
| src/tests/incorrect_description_file.rs | Simplified expected file dependencies and missing-dependencies check |
| src/lib.rs | Replaced manual dot-resolution logic with load_as_file_or_directory |
Comments suppressed due to low confidence (1)
src/lib.rs:503
- [nitpick] Update this comment to reflect that both
.and./are special-cased for directory resolution.
// ensure resolve directory only when specifier is `.`
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 1df7c1a in 54 seconds. Click for details.
- Reviewed
83lines of code in3files - 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. src/tests/incorrect_description_file.rs:20
- Draft comment:
Expected file dependencies now only include the package.json. The test now asserts that only 'pack1/package.json' is registered and missing_dependencies is empty. Confirm this is the intended behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm the intended behavior of a test change. It is not making a specific code suggestion or pointing out a specific issue. This violates the rule against asking the author to confirm their intention.
2. src/tests/resolve.rs:168
- Draft comment:
New test block in 'resolve_dot' verifies that when mainFiles are empty, resolving '.' or './' fails. This aligns with the fix—ensure that a missing mainFiles configuration results in a NotFound error. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what a new test block does and how it aligns with a fix. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
Workflow ID: wflow_bKtkp2xBp25LCc5z
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9d82709 in 1 minute and 1 seconds. Click for details.
- Reviewed
96lines of code in4files - 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/tests/resolve.rs:152
- Draft comment:
The new test block inresolve_dotnicely verifies that when the default mainFiles (e.g. ["index"]) is set, resolving '.' or './' finds the index file, and when mainFiles is empty the dot resolution fails with NotFound. This clearly exercises the intended fix. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/tests/incorrect_description_file.rs:16
- Draft comment:
The tests correctly assert that JSON parse errors update the file dependencies as expected. The explicit equality check on file_dependencies ensures that only the package JSON file is reported. This test appears comprehensive. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/tests/resolve.rs:168
- Draft comment:
Overall, the additional tests (e.g. for normalized Windows paths, preferRelative, and symlink resolution) cover many edge cases. Consider adding a comment or an extra case verifying behavior when custom mainFiles (other than 'index') is provided to further document the intended configuration behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_HcpGcfAKNn1naqHo
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
==========================================
+ Coverage 92.20% 92.22% +0.01%
==========================================
Files 13 13
Lines 3068 3060 -8
==========================================
- Hits 2829 2822 -7
+ Misses 239 238 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #163 will not alter performanceComparing Summary
|
close #160
related #121 and #123, the first argument is
directory, should never befile.Important
Fixes directory resolution for
.and./specifiers to respectmainFilesoption, with updates to tests and documentation.require_relative()insrc/lib.rsto handle.and./specifiers by ensuring directories are resolved only when the specifier is..load_as_file_or_directory()to respectmainFilesoption when resolving directories.resolve_dot()insrc/tests/resolve.rsto test new behavior for.and./specifiers with and withoutmainFiles.incorrect_description_file_1()insrc/tests/incorrect_description_file.rsto check for emptymissing_dependencies.README.mdchangingimport.meta.urltoimport.meta.dirname.This description was created by
for 9d82709. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Documentation
import.meta.urltoimport.meta.dirname.Bug Fixes
Refactor
Tests