Use any color

[KimNorgaard]

I'd like the option to be able to use any color I want.

Currently the number of colors is limited to a few options, but since it's already using fatih/color behind the scenes it should be fairly easy to refactor the code to enable any color fatih/color supports or even color.RGB().

I could hash out a PR if you like the idea.

[chelnak]

Hey @KimNorgaard

This sounds like a great idea.

A PR would be awesome if you have the time.

Thanks!

[KimNorgaard]

So, I might have prematurely proclaimed this an easy fix. Well.. it sort of is, but it will break backwards compatibility. My solution was to completely replace the current abstraction in the colors package with wrappers around fatih/color. But it would be a breaking change.

On the positive side, with this change, it would be possible to use color.New(value ...color.Attribute) to set any attribute such as fg color, bg color, bold, etc.

One way to keep backwards compatibility would be to use generics in the colors package and the color settings functions. This might make it possible to accept both types and possibly deprecate the old types over time.

I would probably also add option methods (like WithSpinnerColor()) to spinnerManager.AddSpinner() making it possible to customize colors for each spinner and defaulting to the spinner managers colors settings.

Let me know what you think.

[KimNorgaard]

Just to give you an idea, I did a quick change here: https://github.com/KimNorgaard/ysmrr/tree/custom-colors

It doesn't support using *color.Color directly from fatih/color but you can use either a colors.Color from ysmrr or a color.Attribute from fatih/color. It's just to give you an idea of the direction it could take. Using *color.Color would probably be better than color.Attribute.

One currently has the disadvantage that you have to create the manager with type assertion (e.g. sm := ysmrr.NewSpinnerManager[colors.Color]() if you are not using any options. I don't know if it's possible for go to infer a 'default' type. But if you use any options, it should infer the type just fine.

[chelnak]

Hey, I took a look at your branch. It looks nice but it's a fairly big change to the public API..

I'm wondering if there is a simpler direction.

Maybe duplicating the colors (or adding a v2) package and providing a whole new set of methods to manage colors. It's not as elegant but allows both to co-exist and the for the first to be safely deprecated.

With regards to exposing color.Color, my original idea was to have an abstraction in place so that the user is not aware of the underlying dependency and also to allow for potential future changes (for example, swapping out the package).

Does that make sense?

[KimNorgaard]

I agree it is a big change and I also don't think it's the best solution. My preferred solution would be to go the v2 route like you wrote because it just seems like too much effort to try to maintain backwards compatibility (and not even be completely able to, anyway).

The abstraction seems sensible as far as not leaking dependencies goes but since it's already tightly coupled to that specific color library, I'm not sure it's necessary. Duplicating all of the colors (and other attributes) would in essence mean duplicating related functionality from the fatih library. I'm not sure I see the gained advantage over just allowing the user to use all of the functionality already present.

Personally I would love to just be able to:

col := color.New(color.FgGreen, color.Bold)
sm := ysmrr.NewSpinnerManager(ysmrr.WithSpinnerColor(col))

Of course this makes it harder to replace fatih/color with something else.

Ultimately it's up to you. I'm happy to got both routes.

[chelnak]

Yeah you have a point regarding duplication. To be honest, the faith package is really good and I have no plans to move away from it soon.

So maybe we go down the v2 route where:

• We leave the original functions in the same place and mark as deprecated
• we introduce new functions that use the faith package as you demonstrate.

Then I think as a follow up, I'd be really interested in looking in to individual spinner overrides.

What do you think?