diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..3b824f6 --- /dev/null +++ b/PR.md @@ -0,0 +1,71 @@ +# fix(convert): zero-pad each channel in RgbToHex + +## Problem + +`RgbToHex` (and its alias `Rgb2hex`) does not zero-pad channel values below +`0x10`, so a single hex digit is emitted for those channels: + +```go +color.RgbToHex([]int{1, 2, 3}) // "123" , want "010203" +color.RgbToHex([]int{0, 0, 0}) // "000" , want "000000" +color.RgbToHex([]int{10, 11, 12}) // "0a0b0c" -> actually "abc" +``` + +This breaks two contracts: + +1. The round-trip with `HexToRgb` is no longer an identity for colors with any + channel below 16: + + ```go + color.RgbToHex(color.HexToRgb("010203")) // "123", not "010203" + ``` + +2. `Rgb2basic` looks a color up in `hex2basicMap`, whose keys are all + fixed-width 6-digit hex strings (e.g. "000000"). Because `RgbToHex(0,0,0)` + returns "000", pure black never matches the fast-path map and silently falls + through to the approximate `RgbToAnsi`. + +The sibling converter `RgbTo256` already builds its hex key with +`fmt.Sprintf("%02x%02x%02x", ...)`, so this is an inconsistency within the same +file. + +## Root cause + +`convert.go`, `RgbToHex`: + +```go +for _, v := range rgb { + hexNodes = append(hexNodes, strconv.FormatInt(int64(v), 16)) +} +``` + +`strconv.FormatInt(_, 16)` produces the minimal representation, i.e. one digit +for values 0-15. + +## Fix + +Format each channel as a fixed two-digit hex value: + +```go +hexNodes = append(hexNodes, fmt.Sprintf("%02x", v)) +``` + +`fmt` is already imported in the file. Existing values >= 16 are unchanged. + +## Test + +Extended `TestRgbToHex` in `utils_test.go` with small-channel cases +(`{0,0,0}`, `{1,2,3}`, `{10,11,12}`) and an explicit +`RgbToHex(HexToRgb(hex)) == hex` round-trip check. + +- Gate: `go test ./` -> ok (PASS) +- `go vet ./...` -> exit 0 +- `go build ./...` -> exit 0 + +Counterfactual: reverting `RgbToHex` to `strconv.FormatInt(int64(v), 16)` makes +the new test fail with `expect: "abc"` vs `actual: "0a0b0c"`. + +Note: `gofmt -l convert.go` reports the file, but this is pre-existing (a doc +comment near line 866, unrelated to this change and present on the base commit). +The changed region is gofmt-clean, so unrelated lines were intentionally left +untouched. diff --git a/convert.go b/convert.go index 21344e7..b57f152 100644 --- a/convert.go +++ b/convert.go @@ -483,7 +483,10 @@ func RgbToHex(rgb []int) string { hexNodes := make([]string, 0, len(rgb)) for _, v := range rgb { - hexNodes = append(hexNodes, strconv.FormatInt(int64(v), 16)) + // each channel must be two hex digits, otherwise channels below 0x10 + // are emitted as a single digit and break both the fixed-width hex + // format and the round-trip with HexToRgb. + hexNodes = append(hexNodes, fmt.Sprintf("%02x", v)) } return strings.Join(hexNodes, "") } diff --git a/utils_test.go b/utils_test.go index b20c033..3b960d2 100644 --- a/utils_test.go +++ b/utils_test.go @@ -119,12 +119,21 @@ func TestRgbToHex(t *testing.T) { {"cccccc", []int{204, 204, 204}}, {"aabbcc", []int{170, 187, 204}}, {"aa99cd", []int{170, 153, 205}}, + // channels below 0x10 must be zero-padded to two hex digits + {"000000", []int{0, 0, 0}}, + {"010203", []int{1, 2, 3}}, + {"0a0b0c", []int{10, 11, 12}}, } for _, item := range tests { assert.Equal(t, RgbToHex(item.given), item.want) assert.Equal(t, Rgb2hex(item.given), item.want) } + + // RgbToHex must be the inverse of HexToRgb, including for small channels. + for _, hex := range []string{"010203", "000000", "0a0b0c", "aabbcc"} { + assert.Equal(t, RgbToHex(HexToRgb(hex)), hex) + } } func TestRgbToAnsi(t *testing.T) {