Open
Conversation
avelino
requested changes
Mar 12, 2026
Owner
avelino
left a comment
There was a problem hiding this comment.
The tri-state idea is good, but there are some issues that block merge:
Blocker: does not compile
BannerSize::NORMALdoes not exist — Rust enums are PascalCase, should beBannerSize::Normal- Same issue in the TOML test:
banner_size = "NORMAL"won't deserialize without#[serde(rename_all)] - Need to align serde with clap. Suggestion: add
#[serde(rename_all = "lowercase")]to the enum and use"normal","compact","none"in both CLI and TOML
Commented-out code
//compact_banner and //let compact_banner = config.compact_banner — remove these, commented-out code shouldn't go into main
Tests
- Missing test for
BannerSize::None(stub withoutecho, no banner) - Missing test for
BannerSize::Compact - Missing test for
compact_bannervsbanner_sizepriority in config
Deprecation warning
--compact-banner is marked as deprecated in the help text, but no warning is emitted when used. Without that, nobody will know they need to migrate. Suggestion:
if compact_banner {
tracing::warn!("--compact-banner is deprecated, use --banner-size compact instead");
}Minor
BannerSize::None can be confused with Option::None — consider Off or Hidden
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here we go. 😉
I've implemented a tri-state option
--banner-sizeto set the banner size:NoneCompactNormal(
--banner-size compactshould be equivalent to--compact-banner)Speaking of which:
I followed the deprecation route, so
--compact-bannercan be removed in two major releases (advise users to move to use--banner-sizeinstead). Of couse, @avelino has the last word, so what he decides goes. 😉To avoid an unnecessary level of complexity,
--compact-banner(CLI) orcompact_banner(configuration) has higher priority over--banner-size(CLI) orbanner_size(configuration), so the removal of the deprecated option can be smooth.Hope it flies! 🚀