Color.from_hsv exception when hue is 360.0

[wconrad]

The docs for Color.from_hsv claim that hue can range from 0 through 360:

# @param [Fixnum] hue The hue component (0-360)

But if a hue of 360 is given:

gem "chunky_png", "1.3.4"
require "chunky_png"
ChunkyPNG::Color.from_hsv(360.0, 1.0, 1.0)

Then an exception is thrown:

/home/wayne/.rvm/gems/ruby-2.2.0/gems/chunky_png-1.3.4/lib/chunky_png/color.rb:199:in `from_hsv': undefined method `map!' for nil:NilClass (NoMethodError)
        from /tmp/chunky.rb:5:in `<main>'

If the docs are correct, then a hue %= 360 in #cylindrical_to_cubic will fix the code. If the code is correct, then the docs should probably indicate that the upper limit is exclusive rather than inclusive. Perhaps using the Ruby range notation of three dots for exclusive and two dots for inclusive would do it:

# @param [Fixnum] hue The hue component (0...360)
# @param [Fixnum] saturation The saturation component (0..1)
# @param [Fixnum] value The value (brightness) component (0..1)

This issue affects .from_hsl as well.

Whichever is wrong, the docs or the code, I'd be glad to create a pull request for it.

[wvanbergen]

What would you consider to be the most logical fix?

[wconrad]

I think the code fix to allow hue of 360 is the most logical fix. Having some ranges be closed and others open could confuse the library's user.

Once the hue is being treated to the modulus operator, you could also eliminate the range check. The doc comment for hue would have something like "Nominal range 0..360. Values outside this range are coerced into this range."

[wvanbergen]

Cool, if you can submit a PR that would be great!

[wconrad]

Sorry about the mess. I had a couple of force pushes to my fork before I got it right. The last one (808c147) is correct.

[wconrad]

I think this is a minor level version bump. Allowing 360 is a bug fix, so that would be a patch level bump. But allowing greater than 360 is an expansion of the API; someone who uses that capability won't work correctly with any prior version of the library.

Have I got that right?

[jhamon]

I would say a hue/lightness 360 should be fixed, but above 360 should still raise an ArgumentError. HSL/HSV are cylindrical color spaces and a hue above 360 doesn't really make sense. If I pass an oddly large value for this param, I think it's likely that I made a mistake and I want it to fail loudly rather than silently continue on using some modulo value.

[wvanbergen]

I think I agree with @jhamon. That is more in line with the rest of the API as well; we never use modulo otherwise.

[wvanbergen]

Fixed by #85