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

Remove deprecated export functions #242

Merged

Conversation

Elad-Laufer
Copy link
Contributor

ToImage() always passes through PNG as it is guaranteed to be supported and lossless

- ToImage always passed through PNG as it is guaranteed to be supported and lossless
@coveralls
Copy link

Coverage Status

Coverage increased (+1.9%) to 78.486% when pulling b0f9586 on wix-incubator:remove-deprecated-exports into ef478ad on davidbyttow:master.

@tonimelisma
Copy link
Collaborator

Is this backwards compatible?

@Elad-Laufer
Copy link
Contributor Author

Is this backwards compatible?

No. I've removed exported functions.

@davidbyttow davidbyttow merged commit e6c6aa0 into davidbyttow:master Jan 8, 2022
@davidbyttow
Copy link
Owner

Thanks @Elad-Laufer !

@tonimelisma
Copy link
Collaborator

tonimelisma commented Jan 9, 2022

Hey @davidbyttow are you sure we want to break backwards compatibility? Or does this mean we should bump up the major version number? If we are to bump the major version number, should we look at what other breaking changes we would want to do simultaneously?

@davidbyttow
Copy link
Owner

davidbyttow commented Jan 9, 2022

Hey @tonimelisma to be honest I didn’t give this too much deep thought. My thinking is that people should be pinning to a specific version, but then again I know that’s not the reality. Perhaps we should revert this and then bump the major (or minor since this has been deprecated for a while now) and then roll forward. Though if we are going to bump the major version then I would like to see if there are other changes to fold in.

WDYT @tonimelisma @Elad-Laufer ?

davidbyttow added a commit that referenced this pull request Jan 9, 2022
davidbyttow added a commit that referenced this pull request Jan 9, 2022
@tonimelisma
Copy link
Collaborator

I've tried to follow Go's backwards compatibility policy pretty strictly. So basically bump up the major version for breaking changes.

I've tried to think about other changes we would want to do simultaneously with a version bump. I wonder where are with regards to supporting all libvips importing and exporting parameters? Would there be breaking changes there we could implement simultaneously?

Might this also be a good time to refactor the export parameters to an alternative that's easier for both us as well as govips users? With the struct we use there's the challenge that we can't see when a user overrides any specific parameters on the govips side. For users, it's a bit more complicated to always get a struct from NewDefaultJPEGExportParams(), fill it in and then pass it back, instead of a simpler variadic interface (such as e.g. here). Not sure if such a change would be worth the effort.

@davidbyttow
Copy link
Owner

Good points, will consider though, but generally agree on the refactor. I need to get my head back into this for a day to form an opinion.

@tonimelisma
Copy link
Collaborator

Sure. Apologies for the lack of availability on my end recently and for the coming weeks, I'm just relocating to SF with my family and there's a lot on my table.

@Elad-Laufer Elad-Laufer deleted the remove-deprecated-exports branch August 3, 2022 15:26
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

4 participants