-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat(rmdir
): introduce
#110
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
base: main
Are you sure you want to change the base?
Conversation
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 introduces a new codemod recipe that migrates deprecated fs.rmdir
calls with recursive: true
to the new fs.rm
API (including Sync and Promise variants) while adding appropriate tests and configuration.
- Adds a workflow definition, TypeScript config, and package manifest for the
rmdir
recipe - Implements and documents the transform in
src/workflow.ts
- Includes input/expected tests and updates
biome.jsonc
to ignore test fixtures
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
recipes/rmdir/workflow.yml | Workflow node for AST transform (step description needs update) |
recipes/rmdir/tsconfig.json | TypeScript compiler options for the recipe |
recipes/rmdir/tests/input/file.js | Test inputs covering rmdir with and without recursive |
recipes/rmdir/tests/expected/file.js | Expected outputs transforming to rm |
recipes/rmdir/src/workflow.ts | Transform implementation for fs.rmdir -> fs.rm |
recipes/rmdir/package.json | Recipe metadata and dependencies |
recipes/rmdir/codemod.yml | Codemod descriptor (name and workflow reference need fixes) |
recipes/rmdir/README.md | Documentation and examples for the recipe |
biome.jsonc | Updated ignore patterns for input and expected dirs |
Comments suppressed due to low confidence (3)
recipes/rmdir/workflow.yml:9
- The step description references an unrelated
assert
import transform. Update it to describe thefs.rmdir
→fs.rm
migration (e.g., "Convert deprecated fs.rmdir recursive calls to fs.rm with force").
- name: "Replace `assert` import attribute to the `with` ECMAScript import attribute."
recipes/rmdir/codemod.yml:2
- The codemod
name
is inconsistent with this recipe. Rename it to something likenodejs/rmdir-to-rm
to reflect the actual transformation.
name: "nodejs/import-assertions-to-attributes"
recipes/rmdir/codemod.yml:7
- The workflow file is named
workflow.yml
but this referencesworkflow.yaml
. Update the filename reference to match (workflow.yml
) so the codemod can find and run the workflow.
workflow: "workflow.yaml"
recipes/rmdir/src/workflow.ts
Outdated
newCallText = `fs.rm(${path}, { recursive: true, force: true }, ${callback})`; | ||
} else { | ||
// No callback | ||
const path = call.getMatch("PATH")?.text(); | ||
newCallText = `fs.rm(${path}, { recursive: true, force: true })`; | ||
} | ||
} else if (callText.includes("fs.rmdirSync(")) { | ||
// Handle fs.rmdirSync -> fs.rmSync | ||
const path = call.getMatch("PATH")?.text(); | ||
newCallText = `fs.rmSync(${path}, { recursive: true, force: true })`; | ||
} else if (callText.includes("fs.promises.rmdir(")) { | ||
// Handle fs.promises.rmdir -> fs.promises.rm | ||
const path = call.getMatch("PATH")?.text(); | ||
newCallText = `fs.promises.rm(${path}, { recursive: true, force: true })`; |
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.
This replacement drops any additional user-specified options besides recursive
. Consider merging existing properties from the original options object with { force: true }
instead of overwriting them.
newCallText = `fs.rm(${path}, { recursive: true, force: true }, ${callback})`; | |
} else { | |
// No callback | |
const path = call.getMatch("PATH")?.text(); | |
newCallText = `fs.rm(${path}, { recursive: true, force: true })`; | |
} | |
} else if (callText.includes("fs.rmdirSync(")) { | |
// Handle fs.rmdirSync -> fs.rmSync | |
const path = call.getMatch("PATH")?.text(); | |
newCallText = `fs.rmSync(${path}, { recursive: true, force: true })`; | |
} else if (callText.includes("fs.promises.rmdir(")) { | |
// Handle fs.promises.rmdir -> fs.promises.rm | |
const path = call.getMatch("PATH")?.text(); | |
newCallText = `fs.promises.rm(${path}, { recursive: true, force: true })`; | |
newCallText = `fs.rm(${path}, { ...${optionsText}, force: true }, ${callback})`; | |
} else { | |
// No callback | |
const path = call.getMatch("PATH")?.text(); | |
newCallText = `fs.rm(${path}, { ...${optionsText}, force: true })`; | |
} | |
} else if (callText.includes("fs.rmdirSync(")) { | |
// Handle fs.rmdirSync -> fs.rmSync | |
const path = call.getMatch("PATH")?.text(); | |
newCallText = `fs.rmSync(${path}, { ...${optionsText}, force: true })`; | |
} else if (callText.includes("fs.promises.rmdir(")) { | |
// Handle fs.promises.rmdir -> fs.promises.rm | |
const path = call.getMatch("PATH")?.text(); | |
newCallText = `fs.promises.rm(${path}, { ...${optionsText}, force: true })`; |
Copilot uses AI. Check for mistakes.
recipes/rmdir/src/workflow.ts
Outdated
// Check if this call has recursive: true option | ||
const optionsMatch = call.getMatch("OPTIONS"); | ||
if (!optionsMatch) continue; | ||
|
||
const optionsText = optionsMatch.text(); | ||
if (!optionsText.includes("recursive") || !optionsText.includes("true")) { | ||
continue; | ||
} |
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.
What about all the other options?
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.
idk if there are an impact on the codemod
recipes/rmdir/src/workflow.ts
Outdated
|
||
let newCallText = ""; | ||
|
||
if (callText.includes("fs.rmdir(")) { |
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.
I don't love all the if...then with repeated logic, can we optimize and refactor a bit?
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.
I should use one AST-grep query + update by function call
Co-Authored-By: Aviv Keller <[email protected]>
i had push shitty commit because to move forward I need new utility |
Related issue
Important
It's use
@next
codemod CLIClose #104
TODO