Skip to content

Quote empty default values in help output#6278

Merged
epage merged 3 commits intoclap-rs:masterfrom
yash27-lab:fix-4976-empty-default-display
Feb 19, 2026
Merged

Quote empty default values in help output#6278
epage merged 3 commits intoclap-rs:masterfrom
yash27-lab:fix-4976-empty-default-display

Conversation

@yash27-lab
Copy link
Contributor

@yash27-lab yash27-lab commented Feb 17, 2026

Summary

  • quote empty default values in help output so they render as "" instead of an empty token
  • keep existing behavior for whitespace-containing values
  • add a derive regression test for default_value_t on String

Fixes #4976

@epage
Copy link
Member

epage commented Feb 17, 2026

Quoting from #6241 (comment)

As noted in #4976 (comment) and https://github.com/clap-rs/clap/blob/master/CONTRIBUTING.md#preparing-the-pr, my expectations for this would be

  • Add any tests that are needed to show the current problem

  • Refactor to consolidate to Escape

  • Update Escape with an is_empty check, updating the tests so they continue to pass

In particular,

  • we prefer tests be added in a prior commit, passing so that they show the current problem. The diff between commits then shows how the behavior changed
  • Refactors should be split out into their own commit
  • For builder-only logic, we should focus on only builder tests and not derive tests

@yash27-lab yash27-lab force-pushed the fix-4976-empty-default-display branch from 4c00d3d to 0e7752e Compare February 17, 2026 20:20
@yash27-lab
Copy link
Contributor Author

Thanks for the guidance. I updated this PR accordingly:\n\n- split into three commits (tests, refactor, then behavior change)\n- moved coverage to builder tests in tests/builder/help.rs\n- refactored escaping through a shared Escape helper\n- added empty-string handling in the final fix commit\n\nPlease take another look when you have a moment.

@yash27-lab yash27-lab force-pushed the fix-4976-empty-default-display branch from 0e7752e to c5a851c Compare February 17, 2026 20:22
@epage
Copy link
Member

epage commented Feb 17, 2026

Please clean up the commits to reflect how this should be reviewed and merged and not the development process.

@yash27-lab yash27-lab force-pushed the fix-4976-empty-default-display branch from d41199a to cc9b169 Compare February 17, 2026 21:14
@yash27-lab yash27-lab force-pushed the fix-4976-empty-default-display branch from cc9b169 to 3076c59 Compare February 17, 2026 22:27
@yash27-lab yash27-lab force-pushed the fix-4976-empty-default-display branch 2 times, most recently from d022748 to b0bd06b Compare February 19, 2026 18:48
@yash27-lab yash27-lab force-pushed the fix-4976-empty-default-display branch from b0bd06b to 4206699 Compare February 19, 2026 18:54
@epage epage merged commit 862fff6 into clap-rs:master Feb 19, 2026
25 checks passed
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.

Default display for String is awkward, ie [default: ]

2 participants

Comments