plotter: Implement XYers interface for polygons #555
Conversation
This makes it possible for external types to implement the interface without copying all data points.
Codecov Report
@@ Coverage Diff @@
## master #555 +/- ##
==========================================
+ Coverage 69.28% 69.29% +0.01%
==========================================
Files 52 52
Lines 5339 5341 +2
==========================================
+ Hits 3699 3701 +2
Misses 1468 1468
Partials 172 172
Continue to review full report at Codecov.
|
2 similar comments
Codecov Report
@@ Coverage Diff @@
## master #555 +/- ##
==========================================
+ Coverage 69.28% 69.29% +0.01%
==========================================
Files 52 52
Lines 5339 5341 +2
==========================================
+ Hits 3699 3701 +2
Misses 1468 1468
Partials 172 172
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #555 +/- ##
==========================================
+ Coverage 69.28% 69.29% +0.01%
==========================================
Files 52 52
Lines 5339 5341 +2
==========================================
+ Hits 3699 3701 +2
Misses 1468 1468
Partials 172 172
Continue to review full report at Codecov.
|
| // Len returns the number of XYers. | ||
| Len() int | ||
|
|
||
| // LenAt returns the length of XYer i. |
There was a problem hiding this comment.
Is there a reason to not define this as
type XYers interface {
// Len returns the number of XYers.
Len() int
// XYer returns the XYer at position i.
XYer(i int) XYer
}
This seems simpler for the user to create if needed.
There was a problem hiding this comment.
My understanding is that if the interface returns plotter.XYer, then every type that implements it also needs to return plotter.XYer (returning a different interface with the same signature is not allowed). So every package that implements this interface would have to import plotter, and the same interface could never really be used for anything else.
This problem is avoided if the interface only includes built-in types.
For my specific use case, I have a general purpose geometry library and I'm trying to make it convenient to plot the polygons, but I don't really want to include plotter as a dependency.
There was a problem hiding this comment.
Yeah, OK. The API is not very nice like this though. I'll let someone else comment. @sbinet?
There was a problem hiding this comment.
just a quick note to say I haven't forgotten about this.
I'll circle back to it before the end of the week.
There was a problem hiding this comment.
I'd be more in favour of Dan's API as well.
I understand you don't want plotter as a dependency of github.com/ctessum/geom, and I agree it isn't great for the separation of concerns (or just the sheer amount of dependencies that come with gonum/plot.)
I also have had that concern and this kind of issue for my hep/hbook package that deals with histograms, but just the statistical aspects of histograms (1D, 2D, ...), not dealing with actually displaying them.
I didn't want hbook to be tied to a specific graphical library having been bitten by this kind of hard dependency in the (C++) particle physics world.
what I did was to create a separate package would depend on hep/hbook and gonum/plot to hold types that would implement the needed interfaces for gonum/plot to work, wrapping hep/hbook values. (it's hep/hplot: https://godoc.org/go-hep.org/x/hep/hplot)
wouldn't this kind of approach (the usual additional indirection switcheroo trick) work for you as well?
There was a problem hiding this comment.
It seems to me like that would result in a worse overall API experience, and I feel like there's a lot to be said for the interfaces only returning built-in types, but if that's the consensus, I'm alright with it I guess.
This makes it possible for external types to implement the interface
without copying all data points.
Fixes #442.
Please take a look.