feat: append --help hint to unknown option errors#712
Conversation
|
Hi @goureeshreddy7 - looks like the branch has fallen behind main since you opened this. Could you rebase on top of the latest main and push? That'll keep the diff clean and unblock the CI checks. Thanks! |
goureeshreddy7
force-pushed
the
feature/cli-help-suggestion
branch
from
d45dc7e to
43f7751
Compare
|
@sonukapoor sir i have succesfully done with rebase and its ready to merge . |
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks for the focused PR. Before diving into the specifics - the catch block in index.ts already appends a --help hint unconditionally on every parseArgs error, which means the UX goal this PR is going for is actually already met today without any changes. That's the starting point worth understanding before deciding what to add here.
A few things to address - details inline. Please also rebase against main first (PR #719 just merged and touches the same error paths). Then reconsider the approach: if the existing catch block hint isn't showing up in the cases you were trying to fix, the right fix is to adjust the render layer - not to embed display copy in thrown errors. Also worth adding tests for the changed paths before resubmitting.
| } | ||
| if (arg.startsWith("-")) { | ||
| throw new Error(`Unknown option: ${arg}`); | ||
| throw new Error(`Unknown option: ${arg} Run \`cve-lite --help\` to see supported options.`); |
There was a problem hiding this comment.
The --help hint belongs at the render layer in index.ts, not embedded in Error.message. Error.message is a programmatic value - callers that catch this error get a string that mixes the machine-readable fact (Unknown option: --foo) with display copy. The catch block in index.ts is already the right place to append the hint, and it already does so. Keep the throw messages clean.
| @@ -74,7 +74,9 @@ try { | |||
| } catch (error) { | |||
| const message = error instanceof Error ? error.message : String(error); | |||
| console.error(chalk.red(`Error: ${message}`)); | |||
There was a problem hiding this comment.
This guard exists because embedding the hint in Error.message above created a duplication problem that now needs papering over. The substring check is also fragile - any future error message that legitimately contains --help (e.g. "--help-url requires a value") will silently suppress the hint. Keeping the throw messages clean removes the need for this guard entirely.
| } | ||
| if (arg.startsWith("-")) { | ||
| throw new Error(`Unknown option: ${arg}`); | ||
| throw new Error(`Unknown option: ${arg} Run \`cve-lite --help\` to see supported options.`); |
There was a problem hiding this comment.
Worth noting: the hint is applied to 3 of the 6 throw sites in args.ts - the config and overrides subcommand blocks are untouched. If the approach were correct, coverage would need to be consistent across all sites.
2 participants
Closes #400
Hi @sonukapoor! As requested in PR #592, I extracted the "--help" suggestion improvement into its own separate PR so it can be tracked independently.
I updated "src/cli/args.ts" to append the help hint directly into the throw message for unknown options and unexpected arguments, and removed the duplicate console log in "src/index.ts" so it prints cleanly. Let me know if everything looks good!