refactor: make cli use ucd-store to download files#35
Conversation
🦋 Changeset detectedLatest commit: 33a23e4 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 |
WalkthroughThe CLI download command was refactored to delegate all Unicode data downloading and filtering logic to the external Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (runDownload)
participant ucd-store
User->>CLI (runDownload): Invoke download command with flags/versions
CLI (runDownload)->>ucd-store: download({ versions, outDir, exclude patterns, etc. })
ucd-store-->>CLI (runDownload): Download results (files downloaded, errors)
CLI (runDownload)-->>User: Output summary (files downloaded, errors)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the objectives from the linked issues. 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/cmd/download.tsOops! Something went wrong! :( ESLint: 9.27.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/cmd/download.test.tsOops! Something went wrong! :( ESLint: 9.27.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. |
luxass
force-pushed
the
refactor_make_cli_use_ucd-store_to_download_files
branch
2 times, most recently
from
e65496b to
917e6fd
Compare
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
I will create an issue for tracking test coverage, which these tests fall under
luxass
force-pushed
the
refactor_make_cli_use_ucd-store_to_download_files
branch
2 times, most recently
from
834f64c to
ce43550
Compare
luxass
force-pushed
the
refactor_make_cli_use_ucd-store_to_download_files
branch
2 times, most recently
from
617b860 to
ce43550
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/cli/test/cmd/download.test.ts (1)
135-135: Follow up on pending test suites.The "file filtering" and "error handling" test suites have been marked as
todo. These tests should be updated to work with the new ucd-store implementation.Would you like me to help update these test suites to work with the new implementation or create an issue to track this work?
Also applies to: 233-233
packages/cli/src/cmd/download.ts (1)
86-93: Test coverage needed for the new implementation.Static analysis indicates these lines lack test coverage. This is expected since the related tests are marked as
todo, but should be addressed.Also applies to: 96-96
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-93: packages/cli/src/cmd/download.ts#L86-L93
Added lines #L86 - L93 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 (5)
.changeset/wise-dodos-fall.md(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/cmd/download.ts(4 hunks)packages/cli/test/cmd/download.test.ts(2 hunks)pnpm-workspace.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- pnpm-workspace.yaml
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/cli/src/cmd/download.ts
[warning] 86-93: packages/cli/src/cmd/download.ts#L86-L93
Added lines #L86 - L93 were not covered by tests
[warning] 96-96: packages/cli/src/cmd/download.ts#L96
Added line #L96 was not covered by tests
🔇 Additional comments (3)
.changeset/wise-dodos-fall.md (1)
1-6: Changeset correctly documents the refactor.The changeset entry properly marks this as a minor version update and clearly describes the migration to ucd-store.
packages/cli/package.json (1)
45-45: Dependency addition looks good.The addition of
@ucdjs/ucd-storeas a workspace dependency aligns with the refactor objective.packages/cli/src/cmd/download.ts (1)
86-93: Clean implementation delegating to ucd-store.The refactored download logic properly delegates all functionality to the
@ucdjs/ucd-storepackage, successfully eliminating code duplication as intended.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-93: packages/cli/src/cmd/download.ts#L86-L93
Added lines #L86 - L93 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/cmd/download.ts(4 hunks)packages/cli/test/cmd/download.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/test/cmd/download.test.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/cli/src/cmd/download.ts
[warning] 85-92: packages/cli/src/cmd/download.ts#L85-L92
Added lines #L85 - L92 were not covered by tests
[warning] 95-95: packages/cli/src/cmd/download.ts#L95
Added line #L95 was not covered by tests
🔇 Additional comments (3)
packages/cli/src/cmd/download.ts (3)
3-7: LGTM! Import updates support the refactor well.The new imports appropriately support the transition to using
@ucdjs/ucd-storefor download functionality and the simplified output formatting.
16-17: LGTM! New flags provide appropriate granular control.The
excludeHTMLFilesandexcludeReadmesflags align well with the new download function interface and maintain user control over file filtering.
35-37: LGTM! Help text properly documents new flags.The help text clearly documents the new exclude flags. The duplicate
--forceflag issue from the previous review appears to have been resolved.
| const totalFiles = result.downloadedFiles.length; | ||
| const totalErrors = result.errors?.length ?? 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM! Simplified summary aligns with the refactor goals.
The streamlined output appropriately reflects that detailed download work is now handled by the external package.
However, the static analysis indicates missing test coverage for the new download logic. Consider adding integration tests to ensure the flag-to-parameter mapping and error handling work correctly with the new @ucdjs/ucd-store dependency.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 95-95: packages/cli/src/cmd/download.ts#L95
Added line #L95 was not covered by tests
🤖 Prompt for AI Agents
In packages/cli/src/cmd/download.ts around lines 95 to 96, add integration tests
to cover the new download logic involving the flag-to-parameter mapping and
error handling with the @ucdjs/ucd-store dependency. Create tests that simulate
different flag inputs and verify the correct parameters are passed to the
download function, as well as tests that trigger and assert proper handling of
errors returned by the download process.
| const result = await download({ | ||
| versions: versions.map(({ version }) => version), | ||
| basePath: outputDir, | ||
| exclude: flags.exclude, | ||
| includeHTMLFiles: !flags.excludeHTMLFiles, | ||
| includeReadmes: !flags.excludeReadmes, | ||
| includeTests: !flags.excludeTest, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the missing force flag parameter.
The refactor successfully simplifies the download logic by delegating to @ucdjs/ucd-store. However, the force flag is defined in the CLI options but not passed to the download function.
Please verify if the download function supports a force parameter:
🏁 Script executed:
#!/bin/bash
# Description: Check if the download function from @ucdjs/ucd-store supports a force parameter
# Expected: Find the function signature and parameter interface
# Search for the download function interface or implementation
ast-grep --pattern $'export function download($$$) {
$$$
}'
# Also search for download function type definitions
rg -A 10 "function download|export.*download" --type tsLength of output: 2161
🏁 Script executed:
#!/bin/bash
# Search for the DownloadOptions definition to see if it includes a `force` field
grep -R "interface DownloadOptions" -n packages/ucd-store/src
grep -R "type DownloadOptions" -n packages/ucd-store/srcLength of output: 187
🏁 Script executed:
#!/bin/bash
# Display the DownloadOptions interface to verify if it includes a `force` field
rg -n -A 15 "interface DownloadOptions" packages/ucd-store/src/download.tsLength of output: 411
Implement or remove the unused force flag
The DownloadOptions interface in @ucdjs/ucd-store does not include a force field, so the CLI’s flags.force is currently ignored. You should either wire up force support end-to-end or remove the flag.
• In packages/cli/src/cmd/download.ts (around lines 85–92):
- Add
force: flags.forceto the call site if you intend to support it.
• In packages/ucd-store/src/download.ts (and itsDownloadOptionsinterface on line 7): - Extend
DownloadOptionsto includeforce?: boolean - Implement the intended “force” behavior (e.g. overwrite existing files).
Example CLI diff:
const result = await download({
versions: versions.map(({ version }) => version),
basePath: outputDir,
exclude: flags.exclude,
includeHTMLFiles: !flags.excludeHTMLFiles,
includeReadmes: !flags.excludeReadmes,
includeTests: !flags.excludeTest,
+ force: flags.force,
});And in ucd-store/src/download.ts:
interface DownloadOptions {
versions: string[];
exclude?: string;
includeTests?: boolean;
includeReadmes?: boolean;
includeHTMLFiles?: boolean;
basePath?: string;
+ force?: boolean;
}📝 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 result = await download({ | |
| versions: versions.map(({ version }) => version), | |
| basePath: outputDir, | |
| exclude: flags.exclude, | |
| includeHTMLFiles: !flags.excludeHTMLFiles, | |
| includeReadmes: !flags.excludeReadmes, | |
| includeTests: !flags.excludeTest, | |
| }); | |
| const result = await download({ | |
| versions: versions.map(({ version }) => version), | |
| basePath: outputDir, | |
| exclude: flags.exclude, | |
| includeHTMLFiles: !flags.excludeHTMLFiles, | |
| includeReadmes: !flags.excludeReadmes, | |
| includeTests: !flags.excludeTest, | |
| force: flags.force, | |
| }); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 85-92: packages/cli/src/cmd/download.ts#L85-L92
Added lines #L85 - L92 were not covered by tests
🤖 Prompt for AI Agents
In packages/cli/src/cmd/download.ts around lines 85 to 92, the download function
call is missing the force flag parameter, which is defined in the CLI options
but not passed through. To fix this, add force: flags.force to the download
function call arguments. Additionally, in packages/ucd-store/src/download.ts,
extend the DownloadOptions interface to include an optional force boolean field
and implement the logic to handle the force behavior, such as overwriting
existing files when force is true.
1 participant
This PR resolves #22
This is done to remove duplicate code.
Summary by CodeRabbit