Implemented use of 3 variables instead of a single

[jonasbn]

Hi @rakyll

I was pondering about my PR #17 and the fact that I work as a product manager as my day job won. The use-case of the GOTEST_PALETTE is not optimal.

So this PR offers an alternative approach. I have however only done the code, not the documentation, but I wanted to now what you think of the approach, before spending too much time on the PR.

The change is splitting the GOTEST_PALETTE into 3 separate environment variables and configurable parameters:

• GOTEST_FAIL_COLOR
• GOTEST_PASS_COLOR
• GOTEST_SKIP_COLOR

The last one, where the use-case originates from my PR #17.

You could keep the GOTEST_PALETTE for backwards compatibility and even apply PR #17 but I think a split as suggested in this PR is a better approach.

Let me know what you think - and if you wanted to evaluate a documentation update, with the code change please let me know.

As I mentioned in PR #17 I am fairly new to Go so if there are code, which could be more idiomatic or something, please let me know.

And I hope it is okay I renamed a few things to keep it more lean.

[subtlepseudonym]

Added a few additional comments regarding variable name and comment changes. They seem like lateral moves in terms of code clarity, so I don't think they're worth causing merge conflicts down the road.

[subtlepseudonym]

I really like this change, but I would implement it a little differently.

You mentioned that you're relatively new to golang and that you're interested in learning idioms / common patterns. I can write a brief implementation in the go playground with some explanations as to why I've made certain design decisions if you're open to that.

[jonasbn]

@subtlepseudonym I would be stupid not to take you up on that offer

I will adjust the PR to focus on the proposed functional change and not so much on my boy scout merrits

[jonasbn]

The PR is not solely focused on the splitting of the single environment variable into three

[subtlepseudonym]

Here's a write up in the go playground of how I was thinking about this change. Let me know if anything is unclear or if I've under- or over-explained anything.
https://play.golang.org/p/lBRrB9-shgG

Additionally, after hacking away at this example for a little while I 100% see the value in the variable name changes (fail, pass, skip) and I apologize for bikeshedding. If you open a separate PR with those changes, I'll approve them.

[jonasbn]

Hi @subtlepseudonym

I am currently in the process of evaluating your suggestions and I have made a separate PR for the variable name changes, please see RP #23.

I can see in the PlayGround write up that you are suggesting working with 3 parameters for the existing use of the environment variable, this is not currently implemented in master, which leads me to - did you see my PR #17, which addresses exactly this?

I really like the possibility of backwards compatibility, so my suggested implementation in PR #17 does not require 3 parameters and the logic is:

if len(vals) > 3 {

Around line 135. Instead of:

if len(vals) != 3 {

As suggested in the PlayGround

What do you think would be the best approach?

[jonasbn]

Based on my comments about the comparison, I revisited PR #17

Did some more testing and updated the documentation.

My recommendation is still for > instead of !=

[subtlepseudonym]

I did see #17 and likely implemented GOTEST_PALETTE that way because that was in the back of my head somewhere. I totally agree with using > over != for backwards compatibility.

[jonasbn]

Updated from master with the latest releases, conflicts resolved

[subtlepseudonym]

Two minor suggestions, otherwise looks good!

[rfyiamcool]

good