Address review feedback: table alignment, refactor to appendFlag, add combination tests

Co-authored-by: arl <476650+arl@users.noreply.github.com>
2026-09-10 07:26:18 -04:00 · 2025-07-31 10:58:34 +00:00 · 2025-07-31 10:58:34 +00:00 · 883c536c88
parent b5ecc964fc
commit 883c536c88
3 changed files with 184 additions and 42 deletions
--- a/README.md
+++ b/README.md
@ -288,15 +288,15 @@ layout: [branch, "|", flags, "|", stats]

 This is the list of additional configuration `options`:

-| Option                          | Description                                                                     |      Default       |
-| :------------------------------ | :------------------------------------------------------------------------------ | :----------------: |
-| `branch_max_len`                | Maximum displayed length for local and remote branch names                      |   `0` (no limit)   |
-| `branch_trim`                   | Trim left, right or from the center of the branch (`right`, `left` or `center`) | `right` (trailing) |
-| `ellipsis`                      | Character to show branch name has been truncated                                |        `…`         |
-| `hide_clean`                    | Hides the clean flag entirely                                                   |      `false`       |
-| `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`       |
+| Option                            | Description                                                                     |      Default       |
+| :-------------------------------- | :------------------------------------------------------------------------------ | :----------------: |
+| `branch_max_len`                  | Maximum displayed length for local and remote branch names                     |   `0` (no limit)   |
+| `branch_trim`                     | Trim left, right or from the center of the branch (`right`, `left` or `center`) | `right` (trailing) |
+| `ellipsis`                        | Character to show branch name has been truncated                               |        `…`         |
+| `hide_clean`                      | Hides the clean flag entirely                                                  |      `false`       |
+| `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`       |

 ## Troubleshooting
--- a/tmux/formater.go
+++ b/tmux/formater.go
@ -297,40 +297,39 @@ 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 {
 	// Handle empty symbol case based on hide_flag_count_if_empty_symbol option
 	if symbol == "" {
 		if f.Options.HideFlagCountIfEmptySymbol {
-			return "" // Hide both symbol and count
+			return flags // Hide both symbol and count
 		}
 		// Show just the count without symbol
-		return fmt.Sprintf("%s%d", style, count)
+		return append(flags, fmt.Sprintf("%s%d", style, count))
 	}

+	// Handle flags_without_count option
 	if f.Options.FlagsWithoutCount {
-		return fmt.Sprintf("%s%s", style, symbol)
+		return append(flags, fmt.Sprintf("%s%s", style, symbol))
 	}
-	return fmt.Sprintf("%s%s%d", style, symbol, count)
+	
+	// Default behavior: show symbol and count
+	return append(flags, fmt.Sprintf("%s%s%d", style, symbol, count))
 }

 func (f *Formater) flags() string {
 	var flags []string
 	if f.st.IsClean {
 		if f.st.NumStashed != 0 {
-			flag := f.formatFlag(f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed)
-			if flag != "" {
-				flags = append(flags, flag)
-			}
+			flags = f.appendFlag(flags, f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed)
 		}

 		if !f.Options.HideClean {
-			// Handle clean symbol separately since it doesn't have a count
+			// Handle clean symbol separately since it doesn't have a meaningful 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)
 			}
+			// Note: When clean symbol is empty, there's nothing meaningful to show
+			// since clean doesn't have a count, so we just skip it
 		}

 		if len(flags) != 0 {
@ -339,38 +338,23 @@ func (f *Formater) flags() string {
 	}

 	if f.st.NumStaged != 0 {
-		flag := f.formatFlag(f.Styles.Staged, f.Symbols.Staged, f.st.NumStaged)
-		if flag != "" {
-			flags = append(flags, flag)
-		}
+		flags = f.appendFlag(flags, f.Styles.Staged, f.Symbols.Staged, f.st.NumStaged)
 	}

 	if f.st.NumConflicts != 0 {
-		flag := f.formatFlag(f.Styles.Conflict, f.Symbols.Conflict, f.st.NumConflicts)
-		if flag != "" {
-			flags = append(flags, flag)
-		}
+		flags = f.appendFlag(flags, f.Styles.Conflict, f.Symbols.Conflict, f.st.NumConflicts)
 	}

 	if f.st.NumModified != 0 {
-		flag := f.formatFlag(f.Styles.Modified, f.Symbols.Modified, f.st.NumModified)
-		if flag != "" {
-			flags = append(flags, flag)
-		}
+		flags = f.appendFlag(flags, f.Styles.Modified, f.Symbols.Modified, f.st.NumModified)
 	}

 	if f.st.NumStashed != 0 {
-		flag := f.formatFlag(f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed)
-		if flag != "" {
-			flags = append(flags, flag)
-		}
+		flags = f.appendFlag(flags, f.Styles.Stashed, f.Symbols.Stashed, f.st.NumStashed)
 	}

 	if f.st.NumUntracked != 0 {
-		flag := f.formatFlag(f.Styles.Untracked, f.Symbols.Untracked, f.st.NumUntracked)
-		if flag != "" {
-			flags = append(flags, flag)
-		}
+		flags = f.appendFlag(flags, f.Styles.Untracked, f.Symbols.Untracked, f.st.NumUntracked)
 	}

 	if len(flags) > 0 {
--- a/tmux/formater_test.go
+++ b/tmux/formater_test.go
@ -1299,6 +1299,164 @@ func TestFlagsWithEmptySymbolsNewBehavior(t *testing.T) {
 	}
 }

+func TestFlagsWithCombinedOptions(t *testing.T) {
+	tests := []struct {
+		name    string
+		styles  styles
+		symbols symbols
+		options options
+		st      *gitstatus.Status
+		want    string
+	}{
+		{
+			name: "flags_without_count=true + hide_flag_count_if_empty_symbol=false, empty symbol shows nothing",
+			styles: styles{
+				Clear:    "StyleClear",
+				Modified: "StyleMod",
+				Stashed:  "StyleStash",
+			},
+			symbols: symbols{
+				Modified: "", // empty symbol
+				Stashed:  "SymbolStash",
+			},
+			options: options{
+				FlagsWithoutCount:              true,
+				HideFlagCountIfEmptySymbol:     false, // Should show count only for empty symbols
+			},
+			st: &gitstatus.Status{
+				NumStashed: 1,
+				Porcelain: gitstatus.Porcelain{
+					NumModified: 2,
+				},
+			},
+			// When flags_without_count=true but symbol is empty, we still get count-only display
+			want: "StyleClear" + "StyleMod2 StyleStashSymbolStash",
+		},
+		{
+			name: "flags_without_count=true + hide_flag_count_if_empty_symbol=true, empty symbol hides everything",
+			styles: styles{
+				Clear:    "StyleClear",
+				Modified: "StyleMod",
+				Stashed:  "StyleStash",
+			},
+			symbols: symbols{
+				Modified: "", // empty symbol
+				Stashed:  "SymbolStash",
+			},
+			options: options{
+				FlagsWithoutCount:              true,
+				HideFlagCountIfEmptySymbol:     true, // Should hide empty symbols completely
+			},
+			st: &gitstatus.Status{
+				NumStashed: 1,
+				Porcelain: gitstatus.Porcelain{
+					NumModified: 2,
+				},
+			},
+			// Empty symbol should be hidden entirely
+			want: "StyleClear" + "StyleStashSymbolStash",
+		},
+		{
+			name: "flags_without_count=false + hide_flag_count_if_empty_symbol=false, mixed symbols",
+			styles: styles{
+				Clear:     "StyleClear",
+				Modified:  "StyleMod",
+				Stashed:   "StyleStash",
+				Untracked: "StyleUntracked",
+			},
+			symbols: symbols{
+				Modified:  "", // empty symbol
+				Stashed:   "SymbolStash",
+				Untracked: "", // empty symbol
+			},
+			options: options{
+				FlagsWithoutCount:              false,
+				HideFlagCountIfEmptySymbol:     false, // Show count for empty symbols
+			},
+			st: &gitstatus.Status{
+				NumStashed: 1,
+				Porcelain: gitstatus.Porcelain{
+					NumModified:  2,
+					NumUntracked: 3,
+				},
+			},
+			// Empty symbols show count only, normal symbols show symbol+count
+			want: "StyleClear" + "StyleMod2 StyleStashSymbolStash1 StyleUntracked3",
+		},
+		{
+			name: "flags_without_count=false + hide_flag_count_if_empty_symbol=true, mixed symbols",
+			styles: styles{
+				Clear:     "StyleClear",
+				Modified:  "StyleMod",
+				Stashed:   "StyleStash",
+				Untracked: "StyleUntracked",
+			},
+			symbols: symbols{
+				Modified:  "", // empty symbol
+				Stashed:   "SymbolStash",
+				Untracked: "", // empty symbol
+			},
+			options: options{
+				FlagsWithoutCount:              false,
+				HideFlagCountIfEmptySymbol:     true, // Hide empty symbols completely
+			},
+			st: &gitstatus.Status{
+				NumStashed: 1,
+				Porcelain: gitstatus.Porcelain{
+					NumModified:  2,
+					NumUntracked: 3,
+				},
+			},
+			// Empty symbols are hidden, normal symbols show symbol+count
+			want: "StyleClear" + "StyleStashSymbolStash1",
+		},
+		{
+			name: "all flags with different combination settings",
+			styles: styles{
+				Clear:     "StyleClear",
+				Staged:    "StyleStaged",
+				Modified:  "StyleMod",
+				Conflict:  "StyleConflict",
+				Stashed:   "StyleStash",
+				Untracked: "StyleUntracked",
+			},
+			symbols: symbols{
+				Staged:    "SymbolStaged",
+				Modified:  "", // empty
+				Conflict:  "SymbolConflict",
+				Stashed:   "", // empty
+				Untracked: "SymbolUntracked",
+			},
+			options: options{
+				FlagsWithoutCount:              true,  // Show symbols without counts
+				HideFlagCountIfEmptySymbol:     false, // But for empty symbols, show count only
+			},
+			st: &gitstatus.Status{
+				NumStashed: 2,
+				Porcelain: gitstatus.Porcelain{
+					NumStaged:    1,
+					NumModified:  3,
+					NumConflicts: 1,
+					NumUntracked: 4,
+				},
+			},
+			// Non-empty symbols show symbol only (flags_without_count=true)
+			// Empty symbols show count only (hide_flag_count_if_empty_symbol=false)
+			want: "StyleClear" + "StyleStagedSymbolStaged StyleConflictSymbolConflict StyleMod3 StyleStash2 StyleUntrackedSymbolUntracked",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := &Formater{
+				Config: Config{Styles: tt.styles, Symbols: tt.symbols, Options: tt.options},
+				st:     tt.st,
+			}
+
+			compareStrings(t, tt.want, f.flags())
+		})
+	}
+}
+
 func compareStrings(t *testing.T, want, got string) {
 	if got != want {
 		t.Errorf(`