rdp / ruby_gnuplot

The ruby gnuplot gem [gnuplot] [rgnuplot] (official releases of the gnuplot gem are from rdp branch)
BSD 3-Clause "New" or "Revised" License
217 stars 48 forks source link

Bugfix for SPlot #16

Closed TheKnight closed 11 years ago

TheKnight commented 11 years ago

Hi, rdp!

In SPlot implementation was find old code remains. They has generate an exception when we used SPlot object. I'm trying to fix this bug, but Ruby is Terra Inkognita for me. I hope i'm fixing it correct.

Yours sincerely TheKnight

P.S:I'm sorry about my English.

rdp commented 11 years ago

I'm unfamiliar with splot, could you give an example of its use? -r

On Sat, Dec 8, 2012 at 9:29 PM, Алексей Заровный notifications@github.comwrote:

Hi, rdp!

In SPlot implementation was find old code remains. They has generate an exception when we used SPlot object. I'm trying to fix this bug, but Ruby is Terra Inkognita for me. I hope i'm fixing it correct.

Yours sincerely TheKnight

P.S:I'm sorry about my English.

You can merge this Pull Request by running:

git pull https://github.com/TheKnight/ruby_gnuplot master

Or view, comment on, or merge it at:

https://github.com/rdp/ruby_gnuplot/pull/16 Commit Summary

  • Fix bug in SPlot.to_gplot implementation.

File Changes

  • M lib/gnuplot.rb (5)

Patch Links

TheKnight commented 11 years ago

File 3d_surface_plot.rb from examples directory.

llrt commented 11 years ago

Tested here the patch from TheKnight and it really worked like a charm. Thanks, TheKnight!

rdp, please do pull his fix.

TheKnight and rdp, the SPlot.to_gplot method is now almost equal to the one from its parent, Plot.to_gplot, except for the line:

@arbitrary_lines.each{|line| io << line << "\n" }

If it's really to be equal, shouldn't this method to_gplot just be ommited in SPlot, as it's been inherited from Plot and shall become just ipsis literis?

TheKnight commented 11 years ago

Leonardo, you're right, we can just inherit this method from Plot, and functionality will be save. But separation of method implementation was making in the longest past and i can not calling reasons it. We just can use "super" as to_gplot implementation and it just work for my current task.

rdp commented 11 years ago

So should we wait for a new patch that just removes the method, or just calls super, or should I merge the patch do you think?

On Tue, Dec 11, 2012 at 8:41 AM, Алексей Заровный notifications@github.comwrote:

Leonardo, you're right, we can just inherit this method from Plot, and functionality will be save. But separation of method implementation was making in the longest past and i can not calling reasons it. We just can use "super" as to_gplot implementation and it just work for my current task.

— Reply to this email directly or view it on GitHubhttps://github.com/rdp/ruby_gnuplot/pull/16#issuecomment-11248303.

llrt commented 11 years ago

rdp,

I would pull TheKnight's fix right now, basically because it solves the main problem from SPlot: it was simply just not working after the refactor. Also, it's thanks to him that we had a first solution, which then pointed the way to go. I think you should honor him and pull it right now.

As for the problem of the implementation that should have been mirrored from Plot, to mend it ASAP I may present in the near future a related pull request. Then you shall merge my patch. No need for waiting for my patch, though, as the problem that will remain after TheKnight's fix will be a minor one, i.e. @arbitrary_lines not working with SPlot.

llrt commented 11 years ago

rdp,

Have you seen my comment above? Will you please pull TheKnight`s fix right now?

rdp commented 11 years ago

should be there in 2.6.1 thanks!