refactor: make URL a positional argument in fetch command#19
Conversation
`alby fetch <url>` is more intuitive than `alby fetch -u <url>`, especially for AI agents that naturally pass the URL as the main param. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/fetch.ts (1)
11-11: Consider a deprecation bridge for--urlto avoid breaking existing scripts.Switching to positional URL is good, but this is a breaking CLI change. Consider accepting
--urlas deprecated for one release and preferring positional when both are supplied.Proposed compatibility diff
.argument("<url>", "URL to fetch") + .option("-u, --url <url>", "DEPRECATED: use positional <url> instead") @@ - .action(async (url, options) => { + .action(async (url, options) => { await handleError(async () => { + const resolvedUrl = url ?? options.url; + if (!resolvedUrl) { + throw new Error("URL is required"); + } const client = await getClient(program); const result = await fetch402(client, { - url: url, + url: resolvedUrl, method: options.method, body: options.body, headers: options.headers ? JSON.parse(options.headers) : undefined, maxAmountSats: options.maxAmount, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/fetch.ts` at line 11, Add a deprecated CLI flag bridge so existing scripts using --url keep working: keep the positional argument declaration (.argument("<url>", "URL to fetch")) and also add an option('--url <url>', 'Deprecated: use positional URL') to the command; in the command handler prefer the positional URL when both are supplied, otherwise fall back to the option value, and emit a single deprecation warning when --url is used (e.g., via console.warn or the logger) that tells users to switch to the positional form and that --url will be removed in the next release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands/fetch.ts`:
- Line 11: Add a deprecated CLI flag bridge so existing scripts using --url keep
working: keep the positional argument declaration (.argument("<url>", "URL to
fetch")) and also add an option('--url <url>', 'Deprecated: use positional URL')
to the command; in the command handler prefer the positional URL when both are
supplied, otherwise fall back to the option value, and emit a single deprecation
warning when --url is used (e.g., via console.warn or the logger) that tells
users to switch to the positional form and that --url will be removed in the
next release.
Summary
alby fetch -u <url>toalby fetch <url>, making the URL a positional argument instead of a required flagTest plan
alby fetch https://example.comworksalby fetchwithout a URL shows a usage error-m,-b,-H,--max-amount) still work🤖 Generated with Claude Code
Summary by CodeRabbit