-
-
Notifications
You must be signed in to change notification settings - Fork 26
[FEATURE] Implement hide_flag_count_if_empty_symbol general option #134
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
Conversation
…entation Co-authored-by: arl <[email protected]>
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.
See comment.
Plus I also want to see tests with combination of the both flag-related options
tmux/formater.go
Outdated
if f.st.NumStashed != 0 && f.Symbols.Stashed != "" { | ||
flags = append(flags, f.formatFlag(f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed)) | ||
if f.st.NumStashed != 0 { | ||
flag := f.formatFlag(f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed) | ||
if flag != "" { | ||
flags = append(flags, flag) | ||
} | ||
} | ||
|
||
if !f.Options.HideClean && f.Symbols.Clean != "" { | ||
flags = append(flags, fmt.Sprintf("%s%s", f.Styles.Clean, f.Symbols.Clean)) | ||
if !f.Options.HideClean { | ||
// Handle clean symbol separately since it doesn't have a count | ||
if f.Symbols.Clean != "" { | ||
flags = append(flags, fmt.Sprintf("%s%s", f.Styles.Clean, f.Symbols.Clean)) | ||
} else if !f.Options.HideFlagCountIfEmptySymbol { | ||
// When symbol is empty but we don't want to hide, there's no count to show for clean flag | ||
// so we just skip it (nothing meaningful to display) | ||
} | ||
} | ||
|
||
if len(flags) != 0 { | ||
return f.Styles.Clear + strings.Join(flags, " ") | ||
} | ||
} | ||
|
||
if f.st.NumStaged != 0 && f.Symbols.Staged != "" { | ||
flags = append(flags, f.formatFlag(f.Styles.Staged, f.Symbols.Staged, f.st.NumStaged)) | ||
if f.st.NumStaged != 0 { | ||
flag := f.formatFlag(f.Styles.Staged, f.Symbols.Staged, f.st.NumStaged) | ||
if flag != "" { | ||
flags = append(flags, flag) | ||
} | ||
} | ||
|
||
if f.st.NumConflicts != 0 && f.Symbols.Conflict != "" { | ||
flags = append(flags, f.formatFlag(f.Styles.Conflict, f.Symbols.Conflict, f.st.NumConflicts)) | ||
if f.st.NumConflicts != 0 { | ||
flag := f.formatFlag(f.Styles.Conflict, f.Symbols.Conflict, f.st.NumConflicts) | ||
if flag != "" { | ||
flags = append(flags, flag) | ||
} | ||
} | ||
|
||
if f.st.NumModified != 0 && f.Symbols.Modified != "" { | ||
flags = append(flags, f.formatFlag(f.Styles.Modified, f.Symbols.Modified, f.st.NumModified)) | ||
if f.st.NumModified != 0 { | ||
flag := f.formatFlag(f.Styles.Modified, f.Symbols.Modified, f.st.NumModified) | ||
if flag != "" { | ||
flags = append(flags, flag) | ||
} | ||
} | ||
|
||
if f.st.NumStashed != 0 && f.Symbols.Stashed != "" { | ||
flags = append(flags, f.formatFlag(f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed)) | ||
if f.st.NumStashed != 0 { | ||
flag := f.formatFlag(f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed) | ||
if flag != "" { | ||
flags = append(flags, flag) | ||
} | ||
} | ||
|
||
if f.st.NumUntracked != 0 && f.Symbols.Untracked != "" { | ||
flags = append(flags, f.formatFlag(f.Styles.Untracked, f.Symbols.Untracked, f.st.NumUntracked)) | ||
if f.st.NumUntracked != 0 { | ||
flag := f.formatFlag(f.Styles.Untracked, f.Symbols.Untracked, f.st.NumUntracked) | ||
if flag != "" { | ||
flags = append(flags, flag) | ||
} |
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 would be much simpler if formatFlag mehod was:
func (f *Formater) appendFlag(flags []string, style, symbol string, count int) []string
It would be called like that for each flag kind:
flags = f.appendFlag(flags, f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed)
and appendFlag method would take care of handling all cases with both flag-related options
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.
Refactored the code to use the suggested appendFlag
method approach in commit 883c536. The method signature is func (f *Formater) appendFlag(flags []string, style, symbol string, count int) []string
as suggested, and it handles both flag-related options.
… combination tests Co-authored-by: arl <[email protected]>
Added comprehensive tests for combination of both flag-related options in commit 883c536. The new |
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.
I think we don't need a new option. So remove it, update doc, PR description and readme.
What we need is to let the possibility for the user to do all combinations, by being clever with the use of the flags_without_count
option.
Remember that the default for flags_without_count is false
Everything is listed below and should be converted to test cases.
case 1 (non-empty symbol)
Symbol=S Count=1
-
flags_without_count: false
prints: "S1" -
flags_without_count: true
prints: "S"
case 2 (empty symbol)
Symbol= Count=1
-
flags_without_count: false
prints: "1" -
flags_without_count: true
prints: ""
case 3 (count is 0)
This remains unchanged and doesn't depend on the value of the flags_without_count option
Always prints ""
| `swap_divergence` | Swaps order of behind & ahead upstream counts | `false` | | ||
| `divergence_space` | Add a space between behind & ahead upstream counts | `false` | | ||
| `flags_without_count` | Show flags symbols without counts | `false` | | ||
| `hide_flag_count_if_empty_symbol` | Hide flag count when symbol is empty (false shows count only) | `false` | |
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.
Rename the flag to:
hide_empty_flags
and realign the markdown table.
Rename variable in Go code
DivergenceSpace bool `yaml:"divergence_space"` | ||
SwapDivergence bool `yaml:"swap_divergence"` | ||
FlagsWithoutCount bool `yaml:"flags_without_count"` | ||
HideFlagCountIfEmptySymbol bool `yaml:"hide_flag_count_if_empty_symbol"` |
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.
Rename based on previous comment
@@ -296,47 +297,64 @@ func (f *Formater) currentRef() string { | |||
} | |||
|
|||
// formatFlag formats a flag with or without count based on the flags_without_count option | |||
func (f *Formater) formatFlag(style, symbol string, count int) string { | |||
func (f *Formater) appendFlag(flags []string, style, symbol string, count int) []string { |
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.
Align the doc string with the new method name. Document what it does since its complexity has increased. Do not be too verbose though
if f.st.NumStaged != 0 { | ||
flags = f.appendFlag(flags, f.Styles.Staged, f.Symbols.Staged, f.st.NumStaged) |
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.
Modify the appendFlag method so as to use the count (here f.s.NumStaged) and return flags as-is if it's 0.
This should be done everywhere appendFlag is used and should make the code cleaner
This PR implements the
hide_flag_count_if_empty_symbol
option to provide users with more granular control over flag display behavior when symbols are empty, addressing the limitations introduced by #132.Problem
Issue #132 introduced the ability to hide flags by setting their symbols to empty strings (
""
), but this was an all-or-nothing approach - it hides both the symbol AND the count. Users who want to see just the count without the symbol (relying on color to distinguish flag types) had no option to do so.Solution
Added a new configuration option
hide_flag_count_if_empty_symbol
that defaults tofalse
, giving users control over this behavior:hide_flag_count_if_empty_symbol: false
(default): When a symbol is empty, show just the styled count (e.g.,3
instead of⚑ 3
)hide_flag_count_if_empty_symbol: true
: Use the current Add ability to hide specific flags by setting their symbols to empty string #132 behavior - hide both symbol and count when symbol is emptyExamples
Before (only one behavior available):
After (user can choose):
Implementation Details
formatFlag()
function to handle empty symbols based on the new optionflags()
function to properly delegate empty symbol handlingTesting
TestFlagsWithEmptySymbolsNewBehavior
verifies the new default behaviorThe feature provides users with the flexibility requested while preserving existing functionality for those who prefer the #132 behavior.
Fixes #133.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.