search

veandco / go-sdl2

SDL2 binding for Go
https://godoc.org/github.com/veandco/go-sdl2
BSD 3-Clause "New" or "Revised" License
2.18k stars 219 forks source link

raster: does this package belong in go-sdl2? #442

Closed jonathaningram closed 4 years ago

jonathaningram commented 4 years ago

While writing #441, and reading the PRs that introduced the Set method, I see that the raster package was also introduced. Looking through the package, I wonder if it's better placed outside of this library as a standalone library or even just in userland code if the calling program needs that behaviour.

The reasons are that it introduces a dependency to github.com/golang/freetype/raster but also raster does not even have a dependency on any github.com/veandco/go-sdl2/* sub packages. This means that raster is not really related to SDL—in fact, as the ImagePainter type explains, you can use it for generic images, not only SDL surfaces: https://github.com/veandco/go-sdl2/blob/3d9b5d6d191123cf3e0ab67158f025e5f0517318/raster/painter.go#L14

I know it doesn't necessarily hurt to have it in here but looking from the outside in, it seems out of place to me. Perhaps I'm missing or misunderstanding something, so happy to be corrected!

veeableful commented 4 years ago

Hi @jonathaningram, I don't think you are missing anything, it is just a result of a green maintainer like me :joy:. At the moment, I thought that it would be convenient and more easily discoverable as this repository contains the commit history and issue that explains its use-case. And I think if you import the other sub-packages directly (e.g. import "github.com/veandco/go-sdl2/sdl"), it shouldn't add freetype/raster as a dependency?

In hindsight, perhaps it should have been inside another repository and we could just add comments or notes that link to that so I apologize if this made the structure look strange! I don't have any strong reason to have it moved outside of this repository.

jonathaningram commented 4 years ago

All good @veeableful it happens and like I said, it doesn't matter and doesn't block me either, but I I happened to notice this so thought I would ask :)

I think you're right about freetype/raster not being pulled in if you only import sub-packages. I stand corrected on that front. I don't know if things change if this library ever gets a go.mod file because at that time the freetype dep would be listed in there.

No need to apologise! I'll go ahead and close this issue as with the other one. It might be something to consider again before any v1.0 release.

veeableful commented 4 years ago

@jonathaningram Oh yeah.. I haven't thought about adding go.mod file at all. I will have a look at it as well. Thank you for reminding me about it and for the question about the package structure! I will consider moving the raster package out for v1.