Skip to content

fix: only detect bg if needed#36

Merged
caarlos0 merged 1 commit intomainfrom
bg
Jun 19, 2025
Merged

fix: only detect bg if needed#36
caarlos0 merged 1 commit intomainfrom
bg

Conversation

@caarlos0
Copy link
Copy Markdown
Member

refs #35

@caarlos0 caarlos0 requested review from Copilot and meowgorithm June 19, 2025 16:15
@caarlos0 caarlos0 self-assigned this Jun 19, 2025
@caarlos0 caarlos0 added the bug Something isn't working label Jun 19, 2025
Copy link
Copy Markdown
Member

@meowgorithm meowgorithm left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR centralizes background detection logic into a new helper, refactors theme initialization in Execute, and cleans up example usage flags.

  • Introduces mustColorscheme to apply a default theme only when needed.
  • Updates Execute in fang.go to use mustColorscheme for help and error styling.
  • Removes explicit WithoutManpage and WithoutCompletions calls from the example.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
theme.go Adds mustColorscheme function and imports os and term.
fang.go Refactors theme logic in Execute to use the new helper.
example/main.go Removes unnecessary theme-related option calls.
Comments suppressed due to low confidence (1)

theme.go:89

  • [nitpick] The function name mustColorscheme is not idiomatic Go; consider renaming to mustColorScheme to follow camelCase conventions for multi-word names.
func mustColorscheme(cs *ColorScheme) ColorScheme {

}
var isDark bool
if term.IsTerminal(os.Stdout.Fd()) {
isDark = lipgloss.HasDarkBackground(os.Stdin, os.Stderr)
Copy link

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

Using os.Stdin and os.Stderr for HasDarkBackground may mismatch the terminal check on os.Stdout. Consider passing the same output stream (e.g., os.Stdout) to ensure consistent background detection.

Suggested change
isDark = lipgloss.HasDarkBackground(os.Stdin, os.Stderr)
isDark = lipgloss.HasDarkBackground(os.Stdout)

Copilot uses AI. Check for mistakes.
@caarlos0 caarlos0 merged commit c4cc657 into main Jun 19, 2025
18 checks passed
@caarlos0 caarlos0 deleted the bg branch June 19, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants