Return value instead of self in attribute writers.

[dlemstra]

As noticed in #585 there are attribute writers that return self instead of the value that was passed in. This PR fixes that for all the attribute writers.

[mockdeep]

So I'm a little confused by this. I think it's probably fine to change, but it appears that Ruby somehow magically does this anyway:

> image = Magick::Image.new(200, 200)
 =>   200x200 DirectClass 16-bit 
> image.background_color=('blue')
 => "blue" 
> image.background_color = 'blue'
 => "blue"

[dlemstra]

Yeah it appears that ruby is doing that itself. I just changed the return type of Info_aset to void but I still get the input value returned. Maybe we should close this PR and change the return type to void instead for all the attribute writers? Can you agree with that change @Watson1978?

[Watson1978]

Hmm, interested. Since Ruby 1.9, Ruby has VM named YARV. it optimize Ruby instruction and it use passed Object as background_color= returned value. I guess it was effect with old Ruby. maybe.

I think we don't need to change anything.

require 'rmagick'

image1 = Magick::Image.new(200, 200)
foo = image1.interlace = Magick::NoInterlace

$ ruby --dump=insns test.rb
== disasm: #<ISeq:<main>@test.rb>=======================================
local table (size: 3, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 3] image1     [ 2] foo
0000 trace            1                                               (   1)
0002 putself
0003 putstring        "rmagick"
0005 opt_send_without_block <callinfo!mid:require, argc:1, FCALL|ARGS_SIMPLE>, <callcache>
0008 pop
0009 trace            1                                               (   3)
0011 getinlinecache   20, <is:0>
0014 getconstant      :Magick
0016 getconstant      :Image
0018 setinlinecache   <is:0>
0020 putobject        200
0022 putobject        200
0024 opt_send_without_block <callinfo!mid:new, argc:2, ARGS_SIMPLE>, <callcache>
0027 setlocal_OP__WC__0 3
0029 trace            1                                               (   4)
0031 putnil
0032 getlocal_OP__WC__0 3
0034 getinlinecache   43, <is:1>
0037 getconstant      :Magick
0039 getconstant      :NoInterlace
0041 setinlinecache   <is:1>
0043 setn             2
0045 opt_send_without_block <callinfo!mid:interlace=, argc:1, ARGS_SIMPLE>, <callcache>
0048 pop
0049 dup
0050 setlocal_OP__WC__0 2
0052 leave

[dlemstra]

Would you be okay with me changing it to void so we have a consistent API?

[Watson1978]

I think we don't need to change anything.

[dlemstra]

We don't need to change anything but I would still like to clean the code up if you are okay with that. The return value is now very mixed throughout the code. And it appears that we no longer need to return something so I would prefer to do this cleanup and change the methods to a void in a new PR.

[Watson1978]

I think this is correct as Ruby theory. So, if we need something, we should merge this implementation.

[dlemstra]

Okay, lets merge this then instead.

[Watson1978]

If you want to merge, you should fix the document at least, I think.

https://github.com/rmagick/rmagick/blob/26e25679521c213b4d6695649d27cb7b781fd78f/ext/RMagick/rmdraw.c#L33

[dlemstra]

The documentation has been updated. Okay to merge now @Watson1978?

[mockdeep]

I guess it's probably not worth changing again at this point, but I kind of think return void would be better in this case to make it clear that the return value doesn't matter, and can't actually be used in any way.

[dlemstra]

I can understand why @Watson1978 prefers to make the method return the value so I don't mind keeping it this way. But you are right it could just be a void instead.