Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix for SPlot #16

Merged
merged 1 commit into from
Dec 28, 2012
Merged

Bugfix for SPlot #16

merged 1 commit into from
Dec 28, 2012

Conversation

TheKnight
Copy link
Contributor

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
Copy link
Owner

rdp commented Dec 10, 2012

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:

#16
Commit Summary

  • Fix bug in SPlot.to_gplot implementation.

File Changes

  • M lib/gnuplot.rb (5)

Patch Links

@TheKnight
Copy link
Contributor Author

File 3d_surface_plot.rb from examples directory.

@llrt
Copy link
Contributor

llrt commented Dec 11, 2012

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
Copy link
Contributor Author

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
Copy link
Owner

rdp commented Dec 12, 2012

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//pull/16#issuecomment-11248303.

@llrt
Copy link
Contributor

llrt commented Dec 12, 2012

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
Copy link
Contributor

llrt commented Dec 28, 2012

rdp,

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

rdp added a commit that referenced this pull request Dec 28, 2012
@rdp rdp merged commit 9ab7de8 into rdp:master Dec 28, 2012
@rdp
Copy link
Owner

rdp commented Dec 28, 2012

should be there in 2.6.1 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants