Open tt opened 11 years ago
@tt hey, are we clear on next steps here? I'm really looking forward to merging this once we're happy with the implementation :)
Hey @tt, would you mind making the changes that @kr suggested and then re-submitting this on heroku/hk?
That will be the canonical home for this project going forward.
Why didn't you reassign the repository ownership then?
I'm reluctant to make the changes. The colorizer is not an io.Writer
. It doesn't handle partial lines properly. If you split the input mid-header, the colorizing doesn't work (the test cases by @kr doesn't cover this). To address this shortcoming, the colorizer needs a buffer which is an unnecessary complexity and a clear violation of the single responsibility principle. We handle complete lines; the LineWriter
interface emphasizes this.`
(Yes, by using the bufio.Scanner
even the io.Writer
gets complete lines, but if the implementation requires this, it should be exposed by the interface. Requiring a caller to apply certain ceremony is asking for trouble.)
@kr, I definitely agree on the bad wording, but I don't agree that SOLID is less applicable to Go just because it's not pure OO. Go in its very design incorporates the Liskov substitution principle in the way you treat interfaces.
Heh, I forgot you could move a repository from one account to another. We could still do that.
Those test cases split the inputs at every combination of boundaries, including mid-header.
Having a buffer in the colorizer is no more complicated than writing a bufio Scanner loop in the main function. Eliminating a special-purpose interface is better than eliminating a few lines of code. Interfaces are generally more expensive to maintain and understand than code.
I argue the colorizer ought to be an io.Writer. It still has a single purpose: to add color escape codes to a log stream in the proper locations.
You could certainly factor it out to a finer grain size, make a LineFilter
io.Writer that takes a func([]byte) []byte
which could colorize complete
lines, but it's just not worth it when there's only one user of the LineFilter.
The parts are ever so slightly smaller, but the whole is more complicated.
While I'm not a huge fan of the arbitrary colors, I like that the existing toolbelt colorizes the log output so you're able to scan it. I ported that. I'm very open to discussing if there's a better way to color it (both to aid readability and with regards to the technical implementation).