fatih / color

Color package for Go (golang)
https://pkg.go.dev/github.com/fatih/color
MIT License
7.24k stars 615 forks source link

Add color.Merge(colors ...*Color) #129

Closed bl-ue closed 3 years ago

bl-ue commented 3 years ago

Adds Merge(colors ...*Color) for merging colors together:

a := color.New(color.FgRed)
b := color.New(color.Bold, color.BgGreen)
c := color.New(color.Italic)
m := color.Merge(a, b, c /* ... */)        // returns a new color without modifying a, b, or c

Fixes #128

apexskier commented 3 years ago

Looks good to me. My only suggestion would be to add a quick test

fatih commented 3 years ago

Thanks for opening the PR. I'm just going over some of the issues in the repo.

I'm not sure exactly whether this makes sense to add. There are many cases for it. The biggest reason is, I don't want to introduce any new features unless it's needed and solving something that can't be done (such as https://github.com/fatih/color/issues/124)

Second, what makes it difficult to use color.Add() or color.New()? Both of them accept variadic arguments, so the example in this PR can be written as:

a := color.New(
    color.FgRed,
    color.Bold, color.BgGreen,
    color.Italic,
)

or:

a := color.New(color.FgRed)
a.Add(color.Bold, color.BgGreen)
a.Add(color.Italic)

Is there a use case that I miss? I've used this library myself in plenty of places. Usually, you define the color.Attributes and then merge them if you need them into a final Color type. Just curious how you think this new function makes things better.

Third, if we assume this API could work, we must find an elegant solution when one of the colors has changed the internal noColor field. This is how the current Color type is defined:

type Color struct {
    params  []Attribute
    noColor *bool
}

As an example, for the following code, should the returned color be false or true for the noColor field?

a := New(FgRed)
a.EnableColor() // assume this is enabled explicitly
b := New(Bold, Italic)
b.DisableColor() // but this is disabled

// What should be m.noColor's value ?
m := Merge(a, b)

Maybe we should assume that this acts like New(), so that field is automatically populated. But we should make it clear how it behaves and improve the doc comment.


Again I appreciate all your help, but I want to make sure we only add something that plays nicely and is needed. This library is already battle-tested in production. So I would not prefer to add another method unless it's required. Thank you.

bl-ue commented 3 years ago

Then maybe should close it. It's sort of nice to have, but not necessary. @apexskier?

apexskier commented 3 years ago

No strong need for this from me. I think @fatih's arguments make sense.

apexskier commented 3 years ago

Thanks for the detailed explanation, and @bl-ue for jumping in!