How should Colors respect Discord's Inconsistencies?

[foxbot]

The behavior of default colors is now inconsistent - we need to ensure that the library respects this.

For roles, a color value of 0 implies to use the default color - this should line up with our current Color structure's Default property.

For embeds, a color value of 0 implies to use black - this does not line up with our current Color structure's Default property. To use the default color, the color field needs to be omitted from the JSON payload.

Our Embed Builder already supports omitting the color field - an embed's Color property is of type Color?. At this point, we just need to make it clear that using Color.Default in an Embed will not actually yield the default color, but will instead produce a black embed.

This should be able to be fixed pretty easily with a Roslyn Analyzer and a documentation string on Color.Default - however, both Analyzers and Documentation are opt-in and not necessarily visible to the user when writing or reading code.

Alternatively, Color.Default could be renamed to Color.RoleDefault, though this may make it confusing to see what should then be used to create a default color in an embed.

Color.Default could also be split between Color.RoleDefault and Color.EmbedDefault, though this does not sound nice in naming, and would require modifying the Color structure to add a boolean or other marker to indicate that the color should actually be null.

[Chris-Johnston]

Would it make sense to make Color an abstract class, then make a new class EmbedColor and RoleColor that have their default values set accordingly in their constructors?

I think that adding a IsSpecifiedflag makes sense, it could be set true for EmbedColor.Default and false for RoleColor.Default. This could be an optional parameter of the constructor which defaults to true. All the other colors would set this flag true.

[AntiTcb]

Alternatively, Color.Default could be renamed to Color.RoleDefault, though this may make it confusing to see what should then be used to create a default color in an embed.

I like this as a half-measure. It's not too hard of a logical step to make that if you want a default color, just omit it when you can see that it can be nulled.

[FiniteReality]

Personally, I think Color.Default should be an uninitialized value, which we handle correctly in the API requests. This way, existing code assuming the correct behaviour does not need to be changed. This way, it is also easy to check against the "default colour" in user code.

[Joe4evr]

@Chris-Johnston Color is currently a struct, though.

[Chris-Johnston]

@Joe4evr Would changing Color to a class affect much?

[FiniteReality]

A simple solution which wouldn't require much effort would be to include a private bool member, which would default to false. Then, in a non-default ctor we'd initialize it to true. For example:

struct Color
{
    public static readonly Color Default
        => default(Color);

    public Color(uint @value)
    {
        _value = @value;
        _initialized = true;
    }

    private bool _initialized;
    private uint _value;

    public uint RawValue
    {
        get { return _value; }
        set { _value = value; _initialized = true; }
    }
}

This way, we can implement something like a GetColorOrDefault method, which returns RawValue if the color is initialized, or a default value if it isn't.

[Joe4evr]

Just FYI, value is not a keyword that needs to be escaped.