Potential toString vs getOriginalInput confusion

[le717]

In having adobe/brackets#9596 reviewed, @redmunds brought out an interesting observation:

(Me) tinycolor.toString() actually works as the author intended. It does not ignore the original case, but rather normalizes (to lowercase) it before returning it. There was actually no way to get the original input (it wasn't even stored) until I suggested it and sent a patch in the form of tinycolor.getOriginalInput().

Yes, but since you added tinycolor.getOriginalInput(), I think it changes expectations. I would expect that method to be called something like toNormalizedString().

I am wondering what your thoughts are on this, @bgrins. I know toString() is a long-standing method and removing/renaming it would be a huge breaking change, but would it be possible to better document the normalization it performs and/or rename getOriginalInput() to something that better describes it's actions vs toString()? I mention renaming as it is a very recent edition to TinyColor and probably isn't used much yet.

[bgrins]

Yes, but since you added tinycolor.getOriginalInput(), I think it changes expectations. I would expect that method to be called something like toNormalizedString().

So the review suggestion is that toString is renamed to toNormalizedString and then rename getOriginalInput to toString? I think changing the behavior of toString wouldn't make sense, since it can take in formats ("hex", "hsl", etc) where the original string has no meaning: https://github.com/bgrins/TinyColor#tostring.

and/or rename getOriginalInput() to something that better describes it's actions vs toString()

Did you have anything in mind?

[redmunds]

@bgrins It just seems like toString() should detect when _originalInput is all upper case and respect that.

[bgrins]

It just seems like toString() should detect when _originalInput is all upper case and respect that.

Given that rule, I see:

• tinycolor("red").toString(): red
• tinycolor("RED").toString(): RED
• tinycolor("#FF0000").toString(): #FF0000

How about given this input?

1. tinycolor("RED").toString("rgb"): rgb(255, 0, 0) or RGB(255, 0, 0)?
2. tinycolor("ReD").toString(): red, ReD, or RED?

[redmunds]

In Brackets, we have a preference for uppercase if that's true, then it's converted to UPPER, otherwise conversions use lower.

So, my biased opinion :) would be that if all alpha chars in original are uppercase use UPPER, otherwise lower.

tinycolor("RED").toString("rgb"): rgb(255, 0, 0) or RGB(255, 0, 0)?

RGB(255, 0, 0)

tinycolor("ReD").toString(): red, ReD, or RED?

red

[le717]

What about adding another parameter to toString(), say, uppercase, that when set to true it returns the content in uppercase (default false, lowercase). That might be easier to implement than upper/lowercase detection and return (which is trivial in itself anyway).

[bgrins]

It's the API design that I'm struggling with here.

I'm afraid implementing this behavior by default in toString would be surprising behavior (if a string is parsed as the same color/format I would expect that calling toString() on it would always return the same thing regardless of the original input). I think this 'expected behavior' may be the point of disagreement.

There could be an extra param to toString or a new function that follows this behavior, but neither option really seems right to me. Which makes me think this may be an application-specific problem/solution. That's my current thinking, at least.

Also, this could be implemented easily outside of the library:

var color = tinycolor("RED");
var str = color.toString();
if (color.getOriginalInput().toUpperCase() === color.getOriginalInput()) {
  str = str.toUppercase();
}

or

tinycolor.prototype.toStringPreservingCase = function() {
  if (this.getOriginalInput().toUpperCase() === this.getOriginalInput()) {
    return this.toString().toUpperCase();
  }
  return this.toString();
}

[bgrins]

Also, this could be implemented easily outside of the library:

I guess maybe not that easy - it would also need to make sure that getOriginalInput is a string before calling toUppercase. But I guess in the case of object input it should default to lower.

[le717]

Also, this could be implemented easily outside of the library:

I had something initially very similar to your examples implemented in the code. It might be better for it to be done that way anyway. I reported this as it was a source of confusion for both me and @redmunds, and I didn't know if this would be something you'd be interested in.

[redmunds]

@bgrins No worries. Thanks for listening to my opinion.

[bgrins]

@le717 @redmunds Thanks for the help on TinyColor and the great work on Brackets :). Going to close this, but feel free to reopen if you'd like to discuss further.