-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Make multi-arg options optional #334
base: main
Are you sure you want to change the base?
Conversation
…gle values Ensure options like capture only one argument, leaving positional arguments intact.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Thank you for this! Can you please verify that multi-arg options are still possible? If so, how does the API distinguish between them? For example, a |
Hey @Pkmmte , I've made the changes based on your feedback. Please check it and let me know if this approach is good or any further changes are required. |
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.
Hi, we've marked this submission as accepted for Hacktoberfest purposes because there's some improvements in here we like, but overall we've found some regressions.
For example, positional arguments no longer work.
npx robo build src/commands/ping.ts -v
For some reason, it treats options (-v
) as positional arguments themselves.
info - Building Robo...
node:internal/fs/promises:1037
const result = await PromisePrototypeThen(
^
Error: ENOENT: no such file or directory, stat '/Users/pkmmte/demo/cli-update-1/-v'
at async Object.stat (node:internal/fs/promises:1037:18)
at async traverse (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/compiler.js:59:18)
at async Object.buildCode (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/compiler.js:133:3)
at async Command.buildAction [as _handler] (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/commands/build/index.js:63:23)
at async Command.processSubCommand (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/cli-handler.js:255:5)
at async Command.processSubCommand (file:///Users/pkmmte/demo/cli-update-1/node_modules/robo.js/dist/cli/utils/cli-handler.js:245:9) {
errno: -2,
code: 'ENOENT',
syscall: 'stat',
path: '/Users/pkmmte/demo/cli-update-1/-v'
}
Node.js v22.2.0
(ENOENT in this case because build
looks for positional arguments in the file system)
Changes expected behavior
The type signature and behavior for multiple options (enabled by default) is also changed. Previously, options with spaces would still yield a single string, but it looks like this now changes to a string array.
While I agree that an array would be more appropriate, we like to avoid changing behavior like this unless it's a major version release. It is likely to break existing code reliant on options always being a single string. In fact, our own create-robo
package would break from this change because of how it uses --plugins
.
As stated earlier, we will be accepting this PR for Hacktoberfest purposes, but may delay in merging due to the above issues.
Hi @Pkmmte, Thank you for your feedback and for accepting the PR for Hacktoberfest. I’ve been working on addressing the issues mentioned, specifically ensuring options like Here’s what I’ve tried so far:
Would you be able to provide additional guidance on addressing this issue? Specifically, are there any expected parsing behaviors or modifications within Thank you for your time and assistance. |
Hello ! thanks for your contribution, I have just tested it out, and it seems to be working just fine for me, are you sure to be testing out the right branch ? |
Hi @Nazeofel, Thanks for testing it out. npx robo build src/cli/commands/add.ts -v
info - Building Robo...
node:internal/fs/promises:1032
const result = await PromisePrototypeThen(
^
Error: ENOENT: no such file or directory, stat '/home/rishi/code/robo.js/packages/robo/-v'
at async Object.stat (node:internal/fs/promises:1032:18)
at async traverse (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils/compiler.js:59:1
8) at async Object.buildCode (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils/compiler
.js:133:3) at async Command.buildAction [as _handler] (file:///home/rishi/code/robo.js/packages/robo/dist/c
li/commands/build/index.js:63:23) at async Command.processSubCommand (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils
/cli-handler.js:255:5) at async Command.processSubCommand (file:///home/rishi/code/robo.js/packages/robo/dist/cli/utils
/cli-handler.js:245:9) { errno: -2,
code: 'ENOENT',
syscall: 'stat',
path: '/home/rishi/code/robo.js/packages/robo/-v'
}
Node.js v22.9.0
But when I removed npx robo build src/cli/commands/add.ts
info - Building Robo...
Type Name Description
───────────────────────────────────────────────
Command Δ /help Displays a list of commands.
Event Δ interactionCreate
Event Δ ready
Δ = Automatically generated
Robo size: 836.92 kB
Built in 113ms
discord:error - DISCORD_TOKEN or DISCORD_CLIENT_ID not found in environment variables
Is this what you are talking about in the previous comment @Pkmmte? |
Hey @Pkmmte, I’ve tested the issue on the main branch as well and observed the following outputs in two cases:
This Thank you! |
Hey ! it should work just fine on the main branch and latest version of Robo, just to confirm somethin, which OS are you running ? |
Ubuntu noble 24.04 x86_64 |
Hey sorry to get back to you so late, we cannot reproduce this issue at all on our environment with the main branch, tested on MacOS, Windows, and Linux too. would you be able to share your all Linux configuration where you are running the command please. |
Summary:
This PR fixes issue #331, where single-value options in the CLI handler were incorrectly consuming positional arguments. The parser now correctly distinguishes between single-value and multi-value options, ensuring options like
-k
or--kit
consume only one argument, while options like--plugins
can accept multiple values.Key Changes:
Updated
Option
Interface:acceptsMultipleValues?: boolean
to indicate if an option accepts multiple values.Modified
option
Method:acceptsMultipleValues
parameter (default isfalse
).Adjusted
parseOptions
Method:--
is encountered.--
as a separator between options and positional arguments.Updated
processSubCommand
Method:parseOptions
to correctly handle positional arguments.Added Tests:
--
separator.Examples of Corrected Behavior:
Single-Value Option:
{ kit: 'activity' }
['myactivity']
Multi-Value Option with Separator:
{ plugins: ['@robojs/ai', '@robojs/sync'] }
['myactivity']
Impact:
Testing:
Notes:
--
to separate multi-value options from positional arguments when necessary.Thank you for reviewing this PR.