Conversation
🦋 Changeset detectedLatest commit: fa61a58 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
WalkthroughThis change refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant LocalUCDStore
participant utils/ucd-files
participant FSAdapter
LocalUCDStore->>utils/ucd-files: mirrorUCDFiles(options)
utils/ucd-files->>FSAdapter: createDefaultFSAdapter()
utils/ucd-files->>FSAdapter: write/read/exists operations
utils/ucd-files-->>LocalUCDStore: Result (mirrored files, errors)
Assessment against linked issues
Assessment against linked issues: Out-of-scope 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
packages/cli/src/cli-utils.tsOops! Something went wrong! :( ESLint: 9.28.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/ucd-store/src/local.tsOops! Something went wrong! :( ESLint: 9.28.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/cli/test/cli-utils.test.tsOops! Something went wrong! :( ESLint: 9.28.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
✨ 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 (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
…ogic to use buildUCDPath
…ment repairUCDFiles functionality with improved error handling and file downloading logic
I will create a new PR which adds a new CLI for stores, that will be the new cli to use
github-actions
bot
added
pkg: cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
packages/cli/src/cli-utils.ts (1)
170-173:⚠️ Potential issueHelp text still advertises removed
downloadcommand
downloadno longer exists, yet it’s still listed in the help table:["download", "Download Unicode data files."],This will confuse users and should be removed (or the command reinstated).
- ["download", "Download Unicode data files."],
🧹 Nitpick comments (10)
.changeset/happy-banks-happen.md (1)
1-8: Changeset summary is too terse for external consumersThe entry only repeats the commit title. Consumers reading release notes won’t know what moved from
ucd-storeintoutils, or that the CLIdownloadcommand was removed.
Consider adding one or two bullet points under the header to spell out:
ucd-storenow delegates file IO / validation to@ucdjs/utils.- CLI:
downloadcommand deprecated (will be replaced bystore).Helps users anticipate behavioural changes without digging through PRs.
packages/utils/test/ucd-files/fs-adapter.test.ts (1)
37-47: Great ↑ coverage, but add explicit assertion count & restore spyNice addition! Two tiny test-hygiene tweaks:
it("should write file successfully", async () => { + expect.assertions(1); // guards against false positives const { writeFile } = await import("node:fs/promises"); @@ - await fs.writeFile("/test.txt", "file content"); - - expect(writeFile).toHaveBeenCalledWith("/test.txt", "file content", "utf-8"); + await fs.writeFile("/test.txt", "file content"); + expect(writeFile).toHaveBeenCalledWith( + "/test.txt", + "file content", + "utf-8", + ); + vi.restoreAllMocks(); // defensive cleanup for later tests });The extra
expect.assertions(1)ensures the spy expectation actually runs; restoring mocks keeps isolation if more tests are appended later.packages/cli/test/cli-utils.test.ts (3)
6-7: Don’t leave commented-out mocksDead code clutters the test file and can easily drift out of date. If the mock will return with the upcoming
storecommand, re-add it when that change lands.
For now, delete the line or annotate with a TODO including a ticket/PR reference.
14-17: Prefertest.skipover commented-out blocksVitest supports skipping while retaining visibility in the test report:
-// it("should return the command from the third positional argument if it is supported", () => { -// const flags: Arguments = { _: ["", "", "download"], version: false }; -// expect(resolveCommand(flags)).toBe("download"); -// }); +it.skip("should return the command from the third positional argument if it is supported (download deprecated)", () => { + const flags: Arguments = { _: ["", "", "download"], version: false }; + expect(resolveCommand(flags)).toBe("download"); +});This keeps CI noise low yet reminds maintainers that coverage is intentionally paused.
80-95: Same for the integration test—useit.skipCommenting removes the test from reporters entirely and risks accidental loss.
Convert to a skipped test or delete until the replacement command is implemented.packages/cli/src/cli-utils.ts (1)
20-22: Minor: expose all supported commands inSUPPORTED_COMMANDS
SUPPORTED_COMMANDSnow only contains"codegen".
Including"help"(and perhaps"version") would makeucd helpresolve via the primary branch instead of the fallback route, improving clarity.Optional tweak:
-const SUPPORTED_COMMANDS = new Set<CLICommand>(["codegen"]); +const SUPPORTED_COMMANDS = new Set<CLICommand>(["help", "version", "codegen"]);packages/utils/test/ucd-files/dump._test.ts (1)
1-210: All tests are commented out – either enable or deleteKeeping a
.tstest file with 100 % commented content adds noise and may confuse tooling.
If these tests are WIP, rename the file to.test.todo.tsor move to a draft branch; otherwise delete it.packages/ucd-store/src/local.ts (2)
100-101: Specify text encoding when reading the manifestRelying on the adapter’s default may return a
Bufferinstead of a UTF-8 string for some implementations.-const manifestContent = await this.#fs.readFile(storeManifestPath); +const manifestContent = await this.#fs.readFile(storeManifestPath, "utf8");🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 100-100: packages/ucd-store/src/local.ts#L100
Added line #L100 was not covered by tests
143-144: Write manifest with explicit encodingSame rationale as above; be explicit.
-await this.#fs.writeFile(storeManifestPath, JSON.stringify(manifestData, null, 2)); +await this.#fs.writeFile( + storeManifestPath, + JSON.stringify(manifestData, null, 2), + "utf8", +);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 143-143: packages/ucd-store/src/local.ts#L143
Added line #L143 was not covered by testspackages/utils/src/ucd-files/validate.ts (1)
223-245: UnboundedPromise.allmay overwhelm the proxyDownloading hundreds of files concurrently can exhaust sockets or throttle the proxy.
Introduce a small pool (e.g.p-limit) or chunk withfor..of+awaitto keep concurrency sane.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-225: packages/utils/src/ucd-files/validate.ts#L223-L225
Added lines #L223 - L225 were not covered by tests
[warning] 228-228: packages/utils/src/ucd-files/validate.ts#L228
Added line #L228 was not covered by tests
[warning] 231-231: packages/utils/src/ucd-files/validate.ts#L231
Added line #L231 was not covered by tests
[warning] 233-233: packages/utils/src/ucd-files/validate.ts#L233
Added line #L233 was not covered by tests
[warning] 235-241: packages/utils/src/ucd-files/validate.ts#L235-L241
Added lines #L235 - L241 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/happy-banks-happen.md(1 hunks)packages/cli/src/cli-utils.ts(1 hunks)packages/cli/src/cmd/download.ts(0 hunks)packages/cli/test/cli-utils.test.ts(2 hunks)packages/cli/test/cmd/download.test.ts(0 hunks)packages/ucd-store/src/download.ts(0 hunks)packages/ucd-store/src/fs-interface.ts(0 hunks)packages/ucd-store/src/index.ts(0 hunks)packages/ucd-store/src/local.ts(8 hunks)packages/ucd-store/src/remote.ts(3 hunks)packages/ucd-store/test/download.fs.test.ts(0 hunks)packages/ucd-store/test/fs-interface.test.ts(0 hunks)packages/utils/src/ucd-files.ts(1 hunks)packages/utils/src/ucd-files/helpers.ts(1 hunks)packages/utils/src/ucd-files/internal.ts(2 hunks)packages/utils/src/ucd-files/validate.ts(3 hunks)packages/utils/test/ucd-files/dump._test.ts(1 hunks)packages/utils/test/ucd-files/fs-adapter.test.ts(1 hunks)pnpm-workspace.yaml(1 hunks)tooling/tsconfig/base.json(1 hunks)
💤 Files with no reviewable changes (7)
- packages/ucd-store/src/index.ts
- packages/cli/test/cmd/download.test.ts
- packages/cli/src/cmd/download.ts
- packages/ucd-store/src/fs-interface.ts
- packages/ucd-store/test/fs-interface.test.ts
- packages/ucd-store/test/download.fs.test.ts
- packages/ucd-store/src/download.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/utils/test/ucd-files/fs-adapter.test.ts (1)
packages/utils/src/ucd-files/fs-adapter.ts (2)
writeFile(33-35)createDefaultFSAdapter(13-58)
packages/cli/test/cli-utils.test.ts (1)
packages/cli/src/cli-utils.ts (1)
resolveCommand(46-56)
packages/utils/src/ucd-files/internal.ts (1)
packages/utils/src/ucd-files.ts (1)
flattenFilePaths(5-5)
🪛 GitHub Check: codecov/patch
packages/ucd-store/src/local.ts
[warning] 72-72: packages/ucd-store/src/local.ts#L72
Added line #L72 was not covered by tests
[warning] 76-76: packages/ucd-store/src/local.ts#L76
Added line #L76 was not covered by tests
[warning] 100-100: packages/ucd-store/src/local.ts#L100
Added line #L100 was not covered by tests
[warning] 119-119: packages/ucd-store/src/local.ts#L119
Added line #L119 was not covered by tests
[warning] 143-143: packages/ucd-store/src/local.ts#L143
Added line #L143 was not covered by tests
[warning] 152-153: packages/ucd-store/src/local.ts#L152-L153
Added lines #L152 - L153 were not covered by tests
packages/utils/src/ucd-files/validate.ts
[warning] 183-196: packages/utils/src/ucd-files/validate.ts#L183-L196
Added lines #L183 - L196 were not covered by tests
[warning] 198-204: packages/utils/src/ucd-files/validate.ts#L198-L204
Added lines #L198 - L204 were not covered by tests
[warning] 206-212: packages/utils/src/ucd-files/validate.ts#L206-L212
Added lines #L206 - L212 were not covered by tests
[warning] 214-216: packages/utils/src/ucd-files/validate.ts#L214-L216
Added lines #L214 - L216 were not covered by tests
[warning] 218-218: packages/utils/src/ucd-files/validate.ts#L218
Added line #L218 was not covered by tests
[warning] 220-220: packages/utils/src/ucd-files/validate.ts#L220
Added line #L220 was not covered by tests
[warning] 223-225: packages/utils/src/ucd-files/validate.ts#L223-L225
Added lines #L223 - L225 were not covered by tests
[warning] 228-228: packages/utils/src/ucd-files/validate.ts#L228
Added line #L228 was not covered by tests
[warning] 231-231: packages/utils/src/ucd-files/validate.ts#L231
Added line #L231 was not covered by tests
[warning] 233-233: packages/utils/src/ucd-files/validate.ts#L233
Added line #L233 was not covered by tests
[warning] 235-241: packages/utils/src/ucd-files/validate.ts#L235-L241
Added lines #L235 - L241 were not covered by tests
[warning] 243-252: packages/utils/src/ucd-files/validate.ts#L243-L252
Added lines #L243 - L252 were not covered by tests
[warning] 254-254: packages/utils/src/ucd-files/validate.ts#L254
Added line #L254 was not covered by tests
[warning] 256-273: packages/utils/src/ucd-files/validate.ts#L256-L273
Added lines #L256 - L273 were not covered by tests
🔇 Additional comments (6)
pnpm-workspace.yaml (1)
24-24:Details
✅ Verification successful
Confirm the new unicode-utils version is published
The alias for
@luxass/unicode-utils-newwas bumped tonpm:@luxass/unicode-utils@0.12.0-beta.8. Ensure this version exists on the registry and matches the utilities your refactor now depends on.
🏁 Script executed:
#!/bin/bash # Verify the published version of unicode-utils npm view @luxass/unicode-utils@0.12.0-beta.8 versionLength of output: 68
✅ Verified: @luxass/unicode-utils@0.12.0-beta.8 is published
Running
npm view @luxass/unicode-utils@0.12.0-beta.8 versionreturned
0.12.0-beta.8, confirming the package exists on the registry. No further action needed.packages/utils/src/ucd-files/helpers.ts (1)
3-17:Details
❓ Verification inconclusive
Edge-case: folders that also carry content are skipped
flattenFilePathsomits the path when an entry haschildren, assuming it’s purely a directory.
If theUnicodeVersionFilemodel ever allows bothchildrenand file data (e.g. an index file inside a folder), that file will be lost.Consider:
- if (file.children) { - paths.push(...flattenFilePaths(file.children, fullPath)); - } else { - paths.push(fullPath); - } + if (file.children?.length) { + paths.push(...flattenFilePaths(file.children, fullPath)); + } + // Push the node itself when it represents a real file (i.e., has no children OR can coexist with children) + if (!file.children || file.children.length === 0 || file.isFile) { + paths.push(fullPath); + }If the data model guarantees “folder OR file” exclusivity, at least document that contract via a JSDoc comment to make the intention explicit.
🏁 Script executed:
#!/bin/bash # Locate the UnicodeVersionFile type definition to verify its shape rg -n "UnicodeVersionFile" -A5 packages/utils/src/ucd-filesLength of output: 2478
Clarify flattenFilePaths Assumptions
- The current implementation treats any node with a defined
childrenarray—even an empty one—as a folder and never pushes its own path.- Please confirm the contract of
UnicodeVersionFile.children:
• Is it only ever defined on directories (and alwaysundefinedon leaf files)?
• Or can a file node have bothchildrenand its own data (or an emptychildrenarray)?- If it’s truly “folder OR file” exclusive, document that in a JSDoc on
flattenFilePaths(or on the type) to make the invariant explicit.- Otherwise, update the code to only recurse when there are actual children and always push the node’s path:
- if (file.children) { - paths.push(...flattenFilePaths(file.children, fullPath)); - } else { - paths.push(fullPath); - } + if (file.children?.length) { + paths.push(...flattenFilePaths(file.children, fullPath)); + } + paths.push(fullPath);Please verify and adjust accordingly.
packages/utils/src/ucd-files/internal.ts (1)
58-62: VerifyflattenFilePathsprefix – risk of path mismatch
flattenFilePaths(filteredEntries, \${version}`)omits theucd/segment that may exist on disk (seeurlPathabove). IfflattenFilePathsdoesn't internally prepend that folder,locatedFiles` will contain paths that cannot be resolved later.Please double-check the helper’s expected prefix and, if necessary, pass the full
urlPath:- locatedFiles.push(...flattenFilePaths(filteredEntries, `${version}`)); + locatedFiles.push(...flattenFilePaths(filteredEntries, urlPath.replace(/^\//, "")));packages/ucd-store/src/remote.ts (1)
93-99: Good swap tobuildUCDPathUsing the shared helper simplifies the URL construction and removes the earlier duplicated logic.
Looks correct and keeps the proxy handling intact.packages/utils/src/ucd-files.ts (1)
5-11: Public surface LGTMRe-exporting
flattenFilePaths,mirrorUCDFiles,repairUCDFiles, and companions provides a clean, central API.
No issues spotted.packages/utils/src/ucd-files/validate.ts (1)
104-104:flattenFilePathsmay return absolute segments – normalise before comparingIf any required path is absolute, the subsequent
files.includes(file)check will fail.
Considerpath.relativeor ensureflattenFilePathsalways yields paths relative to the version root.
| "@ucdjs/utils/ucd-files": ["./packages/utils/src/ucd-files.ts"], | ||
| "@ucdjs/utils/types": ["./packages/utils/src/types.ts"], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coding .ts extensions in path aliases
Pointing a path alias directly at a .ts source file can bite you at runtime if anything consumes the compiled JS without the TypeScript path-mapping (e.g. tests executed with plain node, downstream packages, build artefacts published to npm).
Prefer aliasing a module directory (or the public entry file emitted to dist) instead:
- "@ucdjs/utils/ucd-files": ["./packages/utils/src/ucd-files.ts"],
- "@ucdjs/utils/types": ["./packages/utils/src/types.ts"],
+ "@ucdjs/utils/ucd-files": ["./packages/utils/src/ucd-files"],
+ "@ucdjs/utils/types": ["./packages/utils/src/types"],This leaves resolution to TypeScript during dev while keeping the import specifier extension-agnostic for consumers.
Double-check any IDE/TS-Server errors after the change and ensure the emitted declaration maps stay intact.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@ucdjs/utils/ucd-files": ["./packages/utils/src/ucd-files.ts"], | |
| "@ucdjs/utils/types": ["./packages/utils/src/types.ts"], | |
| "@ucdjs/utils/ucd-files": ["./packages/utils/src/ucd-files"], | |
| "@ucdjs/utils/types": ["./packages/utils/src/types"], |
🤖 Prompt for AI Agents
In tooling/tsconfig/base.json around lines 10 to 11, the path aliases currently
point directly to .ts source files, which can cause runtime issues for consumers
not using TypeScript path mapping. To fix this, change the aliases to point to
the module directories or the public entry files (e.g., the folder or the
compiled output location) instead of specific .ts files. After updating, verify
that IDE/TS-Server does not report errors and that emitted declaration files
maintain correct mappings.
| const fileStructure = await this.getFileTree(version); | ||
| return this.flattenFilePaths(fileStructure); | ||
| return flattenFilePaths(fileStructure); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Possible type / runtime issue when flattening the tree
flattenFilePaths(fileStructure) is called without the optional prefix that other callers supply.
That’s fine only if the second parameter is optional and defaults to "". If not, this will be a TS compile-time error.
Additionally, fileStructure may contain null children because processFileStructure filters only the top level.
flattenFilePaths should defensively skip falsy children to avoid runtime errors.
return flattenFilePaths(
- fileStructure
+ fileStructure.filter(Boolean), // ensure no nulls
+ "" // explicit empty prefix clarifies intent
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fileStructure = await this.getFileTree(version); | |
| return this.flattenFilePaths(fileStructure); | |
| return flattenFilePaths(fileStructure); | |
| } | |
| const fileStructure = await this.getFileTree(version); | |
| return flattenFilePaths( | |
| fileStructure.filter(Boolean), // ensure no nulls | |
| "" // explicit empty prefix clarifies intent | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/ucd-store/src/remote.ts around lines 112 to 114, ensure that when
calling flattenFilePaths(fileStructure), you provide the optional prefix
parameter if it is not defaulted to an empty string to avoid TypeScript errors.
Also, update flattenFilePaths to defensively check for and skip any falsy or
null children in fileStructure to prevent runtime errors caused by null entries
in the tree.
| throw new Error( | ||
| `[ucd-store]: Validation and repair functionality is not implemented yet. Please implement the _validateAndRepairStore method.`, | ||
| ); |
There was a problem hiding this comment.
Unimplemented _validateAndRepairStore blocks shipping code
Throwing here prevents any existing store from booting, effectively breaking consumers. Either:
- Wire this to the new
validateUCDFiles/repairUCDFilesutilities now, or - Mark the entire class as experimental and guard usage upstream.
Leaving a hard throw in production code is unsafe.
Need help integrating the new utilities? Happy to sketch the implementation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 152-153: packages/ucd-store/src/local.ts#L152-L153
Added lines #L152 - L153 were not covered by tests
🤖 Prompt for AI Agents
In packages/ucd-store/src/local.ts around lines 152 to 154, the current code
throws an error because the _validateAndRepairStore method is unimplemented,
which blocks the store from booting and breaks consumers. To fix this, replace
the throw statement by integrating the new validateUCDFiles and repairUCDFiles
utilities within the _validateAndRepairStore method to perform validation and
repair as intended. Alternatively, if immediate integration is not feasible,
mark the class as experimental and add guards upstream to prevent usage in
production until the method is implemented.
| this.#fs = this.#fs || (await createDefaultFSAdapter()); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard createDefaultFSAdapter() only once
Because the constructor already sets #fs, the bootstrap assignment will never execute unless #fs is falsy (now null). The current pattern works but favour a single initialisation site to avoid subtle state bugs; e.g. move the fallback logic to the constructor and drop this line.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 72-72: packages/ucd-store/src/local.ts#L72
Added line #L72 was not covered by tests
🤖 Prompt for AI Agents
In packages/ucd-store/src/local.ts at lines 72-73, the fallback initialization
of #fs using createDefaultFSAdapter() should be removed from bootstrap since the
constructor already sets #fs. Move the fallback logic entirely to the
constructor to ensure #fs is initialized in one place only, then delete the
conditional assignment in bootstrap to avoid redundant or conflicting state
initialization.
| // TODO: fix this! | ||
| this.#fs = options.fs!; | ||
| this.#filter = createPathFilter(filters); |
There was a problem hiding this comment.
Avoid unsafe non-null assertion on options.fs
options.fs! will crash if the caller omits the adapter – precisely the scenario the default path is meant to cover. Either make fs mandatory in the public type or fall back immediately.
-// TODO: fix this!
-this.#fs = options.fs!;
+this.#fs = options.fs ?? null; // initialised later in bootstrap(Remove the non-null assertion and treat the field as possibly null until bootstrap assigns the default.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: fix this! | |
| this.#fs = options.fs!; | |
| this.#filter = createPathFilter(filters); | |
| this.#fs = options.fs ?? null; // initialised later in bootstrap | |
| this.#filter = createPathFilter(filters); |
🤖 Prompt for AI Agents
In packages/ucd-store/src/local.ts around lines 65 to 67, remove the non-null
assertion on options.fs to avoid potential crashes if fs is omitted. Instead,
treat this.#fs as possibly null or undefined initially, and ensure it falls back
to a default adapter during bootstrap or initialization. Adjust the code to
safely handle the absence of fs without assuming it is always provided.
| try { | ||
| // ensure the version directory exists | ||
| await fs.mkdir(versionOutputDir, { recursive: true }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ensureDir instead of mkdir for adapter portability
FSAdapter does not guarantee a low-level mkdir method, but it does expose ensureDir (already used below). Stick to the higher-level helper for consistency.
-// ensure the version directory exists
-await fs.mkdir(versionOutputDir, { recursive: true });
+// ensure the version directory exists
+await fs.ensureDir(versionOutputDir);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // ensure the version directory exists | |
| await fs.mkdir(versionOutputDir, { recursive: true }); | |
| try { | |
| // ensure the version directory exists | |
| await fs.ensureDir(versionOutputDir); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 218-218: packages/utils/src/ucd-files/validate.ts#L218
Added line #L218 was not covered by tests
[warning] 220-220: packages/utils/src/ucd-files/validate.ts#L220
Added line #L220 was not covered by tests
🤖 Prompt for AI Agents
In packages/utils/src/ucd-files/validate.ts around lines 218 to 221, replace the
use of fs.mkdir with fs.ensureDir to create the version directory. This change
ensures compatibility with the FSAdapter, which may not support mkdir but does
provide ensureDir. Update the code to call await fs.ensureDir(versionOutputDir,
{ recursive: true }) instead of mkdir for consistent and portable directory
creation.
1 participant
This PR refactors the ucd-store implementation to use the ucd-files functions from the utils package.
resolves #54
Summary by CodeRabbit