-
-
Notifications
You must be signed in to change notification settings - Fork 715
feat(linter): add cwd property to JS plugin Context
#14814
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
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
CodSpeed Performance ReportMerging #14814 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a cwd (current working directory) property to the JavaScript plugin Context object, making it accessible to custom linter rules. The change threads the cwd parameter through the linting pipeline from the Rust side to the JavaScript plugin context.
Key Changes:
- Added
cwdparameter to the linter'srunandrun_with_disable_directivesmethods - Updated external linter callbacks to accept and pass through the
cwdparameter - Exposed
cwdas a property on the JavaScriptContextclass
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_linter/src/lib.rs | Added cwd parameter to run and run_with_disable_directives methods and passed it to external rule execution |
| crates/oxc_linter/src/external_linter.rs | Updated ExternalLinterLintFileCb type signature to include cwd parameter |
| crates/oxc_linter/src/service/runtime.rs | Updated calls to linter methods to include me.cwd |
| apps/oxlint/src/run.rs | Updated JsLintFileCb type to include cwd in function arguments |
| apps/oxlint/src/js_plugins/external_linter.rs | Updated callback wrapper to accept and forward cwd parameter |
| apps/oxlint/src-js/plugins/lint.ts | Added cwd parameter to lintFile and lintFileImpl functions with validation |
| apps/oxlint/src-js/plugins/context.ts | Added cwd property to InternalContext interface and Context class with getter |
| apps/oxlint/src-js/cli.ts | Updated wrapper function to include cwd parameter |
| tasks/benchmark/benches/linter.rs | Added path as cwd argument to linter.run call |
| napi/playground/src/lib.rs | Added Path::new(".") as cwd argument to linter run call |
| apps/oxlint/test/fixtures/context_properties/plugin.ts | Added test that reports the cwd value |
| apps/oxlint/test/fixtures/context_properties/output.snap.md | Updated snapshot with expected output including cwd reports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
camc314
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not use process.cwd?
passing across the cwd for every file we need to lint feels expensive, especially as the cwd never chanegs (as far as i know)
|
I looked at eslint's implementation, and it just uses process.cwd() (but can be overridden via cli arguments). We don't support CLI arguments, for overriding the current working directory, so we can just use refs: |
@camc314 Thanks for the review! Just to confirm I understand correctly: You're suggesting we use |
Yep exactly! |
camc314
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you!
cwdproperty toContext#14777