-
-
Notifications
You must be signed in to change notification settings - Fork 410
fix: validate and reject duplicate single-value CLI flags #8564
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: unstable
Are you sure you want to change the base?
fix: validate and reject duplicate single-value CLI flags #8564
Conversation
Summary of ChangesHello @Sandijigs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where duplicating single-value Command Line Interface (CLI) flags would silently convert them into arrays, leading to confusing and delayed errors. The changes introduce a robust validation mechanism that proactively detects and rejects such duplicate flags, providing immediate and clear feedback to the user. This significantly improves the user experience by preventing obscure runtime errors related to CLI argument parsing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a validation middleware to prevent duplicate single-value CLI flags, which is a great improvement for user experience. The implementation in command.ts is clean and effective, and the new test suite in command.test.ts is comprehensive. I have a few minor suggestions to improve code style in the error message construction and to make the test assertions more efficient.
| throw new Error( | ||
| `Option '--${optionName}' is a single-value flag but was provided multiple times. ` + | ||
| `Received values: [${value.join(", ")}]. Please provide only one value.` | ||
| ); |
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.
| expect(() => parser.parseSync()).toThrow( | ||
| "Option '--url' is a single-value flag but was provided multiple times" | ||
| ); | ||
| expect(() => parser.parseSync()).toThrow("https://example1.com"); | ||
| expect(() => parser.parseSync()).toThrow("https://example2.com"); |
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.
Calling parser.parseSync() multiple times is inefficient and can be brittle if the function has side effects. It's better to perform a single parse and check the thrown error message for all required substrings. You can combine these assertions into a single toThrow with a more specific error message.
| expect(() => parser.parseSync()).toThrow( | |
| "Option '--url' is a single-value flag but was provided multiple times" | |
| ); | |
| expect(() => parser.parseSync()).toThrow("https://example1.com"); | |
| expect(() => parser.parseSync()).toThrow("https://example2.com"); | |
| expect(() => parser.parseSync()).toThrow( | |
| "Option '--url' is a single-value flag but was provided multiple times. Received values: [https://example1.com, https://example2.com]. Please provide only one value." | |
| ); |
| expect(() => parser.parseSync()).toThrow( | ||
| "Option '--checkpointSyncUrl' is a single-value flag but was provided multiple times" | ||
| ); | ||
| expect(() => parser.parseSync()).toThrow( | ||
| "https://checkpoint-sync.fusaka-devnet-4.ethpandaops.io" | ||
| ); |
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.
Calling parser.parseSync() multiple times is inefficient. It's better to perform a single parse and check the thrown error message. You can combine these assertions into a single toThrow with a more specific error message that includes the duplicated values.
| expect(() => parser.parseSync()).toThrow( | |
| "Option '--checkpointSyncUrl' is a single-value flag but was provided multiple times" | |
| ); | |
| expect(() => parser.parseSync()).toThrow( | |
| "https://checkpoint-sync.fusaka-devnet-4.ethpandaops.io" | |
| ); | |
| expect(() => parser.parseSync()).toThrow( | |
| "Option '--checkpointSyncUrl' is a single-value flag but was provided multiple times. Received values: [https://checkpoint-sync.fusaka-devnet-4.ethpandaops.io, https://checkpoint-sync.fusaka-devnet-4.ethpandaops.io]. Please provide only one value." | |
| ); |
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.
please make sure your code passes yarn build, yarn lint and yarn check-types
Motivation
Duplicating single-value CLI flags (like --checkpointSyncUrl)
by accident gets silently turned into an array, leading to
confusing errors later on. This PR catches duplicate single-value flags early and shows
users exactly what went wrong, instead of letting the error
happen later with a confusing message.
Description
This PR implements validation middleware to detect and
reject duplicate single-value CLI flags with clear error
messages.
Changes made:
Closes #8178
Steps to test or reproduce
yarn vitest run packages/utils/test/unit/command.test.tsShould show 10 tests passing.
Test with actual CLI (reproduces the original
bug):bash ./lodestar beacon --checkpointSyncUrl https://example1.com --checkpointSyncUrl https://example2.comBefore fix: Gets confusing error later("HttpClient.urls[0] is empty")After fix: Gets immediate clear error: Option
--checkpointSyncUrl' is a single-value flag but was provided multiple timesTest that array flags still work:
./lodestar beacon --bootnodes=node1 --bootnodes=node2