Thank you for providing a Go package for the stackblur algorithm.
I'm currently using version v1.0.1, and wanted to update to the newer v1.0.2. When doing that, I ran into a problem, and so I wanted to report it in this issue for your consideration. There are probably different paths to deal with this, and at the end of this issue I provide a suggested resolution.
The API of [email protected] as seen here defines Process:
func Process(src image.Image, radius uint32) image.Image
But the API of [email protected] (seen here) breaks SemVer rule 8 by introducing a backwards incompatible change (the Process function is no longer provided) without also incrementing the major version number.
I also preferred the API of v1.0.1 more than v1.0.2 because it made the input parameter easier to understand: it just needed to implement the image.Image interface. In v1.0.2, the Run function takes input interface{}, and for a user who reads the function's documentation:
// Run takes an image or pixel data as input and returns
// it's blurred version by applying the blur radius defined as parameter.
func Run(input interface{}, radius uint32) (image.Image, error)It might not be clear what types of input are allowed (interface{} doesn't communicate any restrictions, and the comment doesn't elaborate beyond "image or pixel data"). It requires users to read source code or to do trial and error, and it's hard to be confident it won't stop working in a future version.
(However, APIs in both v1.0.1 and v1.0.2 are definitely an improvement over the v1.0.0 API.)
Proposed Path Forward
Resolve SemVer violation
Each of v1.0.0, v1.0.1 and v1.0.2 offers a different, incompatible interface, so users of this package need to modify their code whenever moving between its patch versions. It can become especially problematic if someone's project has dependencies that themselves use different versions of this API.
It's normal to change APIs in order to improve them during v0, before a package reaches version v1.0.0, but it should be avoided after that since the purpose of v1.0.0 (and future minor and patch revisions) is to say "this API is meant to be stable".
If you think this package is ready to commit to a stable API and you just need to pick which one it is, Go 1.16 introduced module retraction. So it would be possible to:
retract v1.0.0 // provides an API that isn't optimal for v1
retract v1.0.1 // makes a backwards incompatible change
retract v1.0.2 // also makes a backwards incompatible change
While publishing a newer version, such as v1.1.0, that provides an API that you expect will be stable going forward (for v1 major version). (Unfortunately, I don't believe it's possible to go back to v0 once a v1+ version is published.)
API design
I would suggest considering the following API design that tries to take the best from v1.0.1 and v1.0.2:
func _(src image.Image, radius int) (*image.NRGBA, error)
That is, the src parameter type is image.Image. If the caller wants to read and decode an image from disk, it's probably better for them to do it themselves (while having full control over how it's done, and how errors are handled), rather than depending on a package that provides a blur algorithm to do it. Trying to accept interface{} makes understanding the API harder without providing substantial benefits that can be depended on.
As another change, consider returning *image.NRGBA (a concrete type that stackblur uses internally anyway) instead of the image.Image interface. The *image.NRGBA concrete type already implements the image.Image interface, so callers that want that can use it trivially. But callers that happen to also use *image.NRGBA for image processing themselves can skip the overhead (or unsafe conversion code) of getting back from image.Image to a concrete type. (See this post and https://go.dev/s/style#interfaces for a more elaborated motivation.)
Finally, you could use int for radius since it's more common/simpler and sufficient—if radius < 1 an error will be returned. But fine to keep using uint32 too.
The function name can be something new, which would allow preserving backwards compatibility with Process from one of v1.0.0 or v1.0.1 (not both since they differ), and/or with Run from v1.0.2, while marking them as deprecated. Or it can be the same name if the earlier versions are retracted anyway.
Thanks very much for considering this issue! I'm happy to discuss this further. If there's agreement on what to do and you'd find it helpful, I can work on creating PRs for review, otherwise please feel free to resolve this as you see fit. Thanks again!
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent