Skip to content

fix: add actionable guidance to flag incompatibility errors (fixes #401)#746

Open
mayank-dev-15 wants to merge 1 commit into
OWASP:mainfrom
mayank-dev-15:fix/better-flag-error-messages
Open

fix: add actionable guidance to flag incompatibility errors (fixes #401)#746
mayank-dev-15 wants to merge 1 commit into
OWASP:mainfrom
mayank-dev-15:fix/better-flag-error-messages

Conversation

@mayank-dev-15

Copy link
Copy Markdown

Error messages now explain what each flag does and suggest which one to use.

Before:
nError: --fix cannot be used with --json Error: --offline/--offline-db cannot be used with --osv-url Error: --no-cache cannot be used with --offline or --offline-db n
After:
nError: --fix cannot be used with --json. Use --fix to apply fixes interactively, or --json to output scan results as JSON - not both at once. Error: --offline/--offline-db cannot be used with --osv-url. Choose offline mode (local DB) or online mode (custom OSV endpoint), not both. Error: --no-cache cannot be used with --offline or --offline-db. In offline mode the local advisory DB is used directly; --no-cache only applies to online scans. n
All 5 error messages updated in src/cli/validate.ts. Existing tests pass (they use substring matching).

Fixes #401

…ASP#401)

Error messages now explain what each flag does and suggest which one to use:

- --offline/--offline-db vs --osv-url: choose offline or online mode
- --no-cache vs --offline: cache only applies to online scans
- --fix vs --json: interactive fixes or JSON output, not both
- --create-pr vs --json: PR or JSON output, not both
- --report vs --json: HTML report or JSON output, pick one

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guidance descriptions are a real improvement over the bare "cannot be used with" messages - the no-cache one especially, since that relationship isn't obvious. One more thing: the new guidance text isn't actually exercised by the existing tests. Jest's toThrow(string) does substring matching, so toThrow("--fix cannot be used with --json") still passes - but a future edit to the guidance prose won't be caught at all. Worth updating a couple of the assertions to cover something distinctive from the new text, like toThrow("apply validated fixes") or toThrow("local advisory DB").

Comment thread src/cli/validate.ts
throw new Error("--fix cannot be used with --json");
throw new Error(
"--fix cannot be used with --json. " +
"Use --fix to apply fixes interactively, or --json to output scan results as JSON — not both at once."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things on this line. interactively isn't accurate for --fix - the flag applies validated direct fixes and rescans without any prompting. Worth updating to match the help text: "Use --fix to apply validated fixes and rescan..."

Also there's an em dash before "not both at once" - project style uses hyphens throughout, so that should be a hyphen or just "pick one."

@sonukapoor

Copy link
Copy Markdown
Collaborator

Hey @mayank-dev-15 — this branch is now behind main. Could you rebase against main and push when you get a chance? Once that's done and the changes from the last review are addressed, we can move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: incompatible flag errors should explain which option to use, not just state the conflict

2 participants