code review 4557047: bike/shed: new package.

bike/shed: new package.

It comes up often enough that it's time to provide
the utility of a standard package.

[dsymonds, 2011-05-25 01:47:09]

Hello r@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[aam, 2011-05-25 01:50:41]

Chartreuse?

[r, 2011-05-25 01:51:24]

http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/Makefile
File src/pkg/bike/shed/Makefile (right):

http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/Makefile#ne...
src/pkg/bike/shed/Makefile:9: colours.go\
use amerikan spelling please.

http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/colours.go
File src/pkg/bike/shed/colours.go (right):

http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/colours.go#...
src/pkg/bike/shed/colours.go:11: // A Colour represents a colour, or a color if
you're not colonial enough.
amerikan spelling please. invert the colonialism.

http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/colours.go#...
src/pkg/bike/shed/colours.go:19: Purple
need Cyan

[r, 2011-05-25 01:54:13]

On Wed, May 25, 2011 at 11:50 AM, andrey mirtchovski
<mirtchovski@gmail.com> wrote:
> Chartreuse?

Don't be counterproductive.

-rob

[dsymonds, 2011-05-25 01:55:11]

I've made the changes, but honestly I don't think there's enough focus
on the bigger picture. Can we move off the minutiae?


Dave.

[adg, 2011-05-25 01:56:06]

On 25 May 2011 11:55, David Symonds <dsymonds@golang.org> wrote:
> I've made the changes, but honestly I don't think there's enough focus
> on the bigger picture. Can we move off the minutiae?

I think it should be shed/bike, as we're more likely to support other
kinds of sheds.

[dsymonds, 2011-05-25 01:57:02]

On Wed, May 25, 2011 at 11:55 AM, Andrew Gerrand <adg@golang.org> wrote:

> On 25 May 2011 11:55, David Symonds <dsymonds@golang.org> wrote:
>> I've made the changes, but honestly I don't think there's enough focus
>> on the bigger picture. Can we move off the minutiae?
>
> I think it should be shed/bike, as we're more likely to support other
> kinds of sheds.

But then the symbols won't read as nicely. bike.Red doesn't make sense
if it's the colour of a shed.


Dave.

[aam, 2011-05-25 02:02:17]

you need tests. preferably ones that cover color blindness. for cyan,
since it isn't covered properly on regular monitors, i suggest you
incorporate the following somehow:

http://i.imgur.com/rWcYi.png

[rsc, 2011-05-25 02:15:32]

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:15: Red Color = iota
These should really be RGBA values.
Also the Color type should implement image.Color.

[niemeyer, 2011-05-25 03:29:25]

According to the contribution guidelines, a CL isn't the right
place to debate about the overall plan.  Preliminary design
discussion should take place in the mailing list first:

    http://golang.org/doc/contribute.html

[adg, 2011-05-25 03:32:56]

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:4: /*
please use // comments here

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:12: type Color int8
uint8?

[r, 2011-05-25 04:13:49]

this cl has little hope of converging.  how can we move it forward?

-rob

[ality, 2011-05-25 04:13:57]

The package name is too Anglo-centric.  I would prefer "sheik/bed".

  Anthony

[aam, 2011-05-25 04:25:23]

> how can we move it forward?

two words: committee.

[dsymonds, 2011-05-25 04:29:07]

I'm just about prepared to make an executive decision on this. Someone else can
push through ISO to get something more widely supported.

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
On 2011/05/25 03:32:56, adg wrote:
> 2011

Done.

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:4: /*
On 2011/05/25 03:32:56, adg wrote:
> please use // comments here

Nah, these are nicer for multiline comments. I might add some example code in
here.

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:12: type Color int8
On 2011/05/25 03:32:56, adg wrote:
> uint8?

Done.

http://codereview.appspot.com/4557047/diff/10001/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:15: Red Color = iota
On 2011/05/25 02:15:32, rsc wrote:
> These should really be RGBA values.
> Also the Color type should implement image.Color.

This is a much more fundamental package. I'd expect that the image package will,
sooner or later, need to import bike/shed.

[peterGo, 2011-05-25 04:34:28]

Why does the color matter? I just want to know if it's going to be big enough
for my penny-farthing.

[r, 2011-05-25 04:44:44]

http://codereview.appspot.com/4557047/diff/16002/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/16002/src/pkg/bike/shed/colors.go#...
src/pkg/bike/shed/colors.go:2: // Use of this source code is governed by a
BSD-style
dropped some copyright

[dsymonds, 2011-05-25 04:46:46]

Oops, fixed.

[r, 2011-05-25 04:48:06]

LGTM

[adg, 2011-05-25 04:49:36]

NOT LGTM

I have further comments.

On 25 May 2011 14:48, Rob 'Commander' Pike <r@golang.org> wrote:
> LGTM
>

[adg, 2011-05-25 04:51:52]

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go#n...
src/pkg/bike/shed/colors.go:13: type Color uint8
There should be a Complement method on Color to return the color's complement,
so that we can take the guesswork out of painting two-toned sheds in an
aesthetically-pleasing way.

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go#n...
src/pkg/bike/shed/colors.go:16: Red Color = iota
s/iota/1 << iota/

So that we can use these bitmasks.

[dsymonds, 2011-05-25 04:53:04]

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go#n...
src/pkg/bike/shed/colors.go:13: type Color uint8
On 2011/05/25 04:51:52, adg wrote:
> There should be a Complement method on Color to return the color's complement,
> so that we can take the guesswork out of painting two-toned sheds in an
> aesthetically-pleasing way.

That can be added later, as the need actually arises.

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go#n...
src/pkg/bike/shed/colors.go:16: Red Color = iota
On 2011/05/25 04:51:52, adg wrote:
> s/iota/1 << iota/
> 
> So that we can use these bitmasks.

I don't think they combine that way, and will artificially limit us to 64
colours on a 64-bit machine.

[r, 2011-05-25 04:53:28]

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go#n...
src/pkg/bike/shed/colors.go:13: type Color uint8
that's really stupid.

[lstoakes, 2011-05-25 08:17:16]

On 25 May 2011 02:51,  <r@golang.org> wrote:

>
http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/Makefile#ne...
> src/pkg/bike/shed/Makefile:9: colours.go\
> use amerikan spelling please.
>
> http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/colours.go
> File src/pkg/bike/shed/colours.go (right):
>
>
http://codereview.appspot.com/4557047/diff/5001/src/pkg/bike/shed/colours.go#...
> src/pkg/bike/shed/colours.go:11: // A Colour represents a colour, or a
> color if you're not colonial enough.
> amerikan spelling please. invert the colonialism.

Being British I clearly disagree, can we please duplicate for both
Color and Colour? We have had a royal wedding recently, you know.

-- 
Lorenzo Stoakes
http://www.codegrunt.co.uk

[iant2, 2011-05-25 13:49:34]

Lorenzo Stoakes <lstoakes@gmail.com> writes:

> Being British I clearly disagree, can we please duplicate for both
> Color and Colour? We have had a royal wedding recently, you know.

Not a rouyal wedding?

Ian

[jnml, 2011-05-25 14:38:35]

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go#n...
src/pkg/bike/shed/colors.go:13: type Color uint8
What is the value of Color type good for w/o a well designed Painter interface?
Anybody out there who wants to propose the drawing primitives?

[bsiegert, 2011-05-26 11:33:20]

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go
File src/pkg/bike/shed/colors.go (right):

http://codereview.appspot.com/4557047/diff/9002/src/pkg/bike/shed/colors.go#n...
src/pkg/bike/shed/colors.go:13: type Color uint8
If Color was an image.Color, we could have transparent sheds by using an
image.RGBAColor. That would be awesome.

[rh, 2011-06-05 09:10:47]

*** Submitted as
http://code.google.com/p/go/source/detail?r=b1k35h3dc01r ***

bike/shed: new package.

It comes up often enough that it's time to provide
the utility of a standard package.

R=r, aam, adg, rsc, niemeyer, ality, peterGo, lstoakes, iant2, jnml, bsiegert
CC=golang-dev
http://codereview.appspot.com/4557047

[rog, 2011-06-05 11:55:31]

sorry, i'm a bit late on this, but can't we have stripes?

On 5 June 2011 10:10,  <robert.hencke@gmail.com> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=b1k35h3dc01r ***
>
> bike/shed: new package.
>
> It comes up often enough that it's time to provide
> the utility of a standard package.
>
> R=r, aam, adg, rsc, niemeyer, ality, peterGo, lstoakes, iant2, jnml,
> bsiegert
> CC=golang-dev
> http://codereview.appspot.com/4557047
>
> http://codereview.appspot.com/4557047/
>

[dsymonds1, 2011-06-09 07:25:37]

I think we're close to consensus.

I've added a TODO for the features people wanted. Anything else before I submit
this?

[r, 2011-06-09 07:32:26]

you need tests.

[jan_mercl, 2011-06-09 07:34:37]

On 2011/06/09 07:25:37, dsymonds1 wrote:
> I think we're close to consensus.
> 
> I've added a TODO for the features people wanted. Anything else before I
submit
> this?

Ponies :-)

LAGTM // Awesomelly

[dsymonds, 2011-06-09 07:36:14]

On Thu, Jun 9, 2011 at 5:32 PM,  <r@golang.org> wrote:

> you need tests.

Done.

[dsymonds, 2011-06-09 07:36:36]

On Thu, Jun 9, 2011 at 5:34 PM,  <befelemepeseveze@gmail.com> wrote:

> Ponies :-)

Ponies are already in the TODO.

[bsiegert, 2011-06-09 10:00:19]

On Thu, Jun 9, 2011 at 09:36, David Symonds <dsymonds@golang.org> wrote:
> On Thu, Jun 9, 2011 at 5:34 PM,  <befelemepeseveze@gmail.com> wrote:
>
>> Ponies :-)
>
> Ponies are already in the TODO.

Hmm. If we want to generalize to a pony shed later, let's split this
package into shed and shed/bikeshed. What do you think?

--Benny.

[kevlar, 2011-06-09 18:16:56]

http://codereview.appspot.com/4557047/diff/31001/src/pkg/bike/shed/colors_tes...
File src/pkg/bike/shed/colors_test.go (right):

http://codereview.appspot.com/4557047/diff/31001/src/pkg/bike/shed/colors_tes...
src/pkg/bike/shed/colors_test.go:14: }
Test for blue-yellow colorblindness.  Suggest making a "colorblindTests" table
to simplify the logic.

[dsymonds, 2011-06-10 12:32:27]

What the heck. It's a long weekend.

[dsymonds, 2011-06-10 12:32:55]

*** Submitted as http://code.google.com/p/go/source/detail?r=8818ac606e92 ***

bike/shed: new package.

It comes up often enough that it's time to provide
the utility of a standard package.

R=r, mirtchovski, adg, rsc, n13m3y3r, ality, go.peter.90, lstoakes, iant,
jan.mercl, bsiegert, robert.hencke, rogpeppe, befelemepeseveze, kevlar
CC=golang-dev
http://codereview.appspot.com/4557047

[peterGo, 2011-06-10 15:27:10]

I only have black and white paint and it doesn't handle grayscale!