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

Merge Filter and AdaptiveFilter enums in the public API #505

Open
Shnatsel opened this issue Sep 26, 2024 · 2 comments
Open

Merge Filter and AdaptiveFilter enums in the public API #505

Shnatsel opened this issue Sep 26, 2024 · 2 comments

Comments

@Shnatsel
Copy link
Contributor

Right now the API for encoding is kinda awkward in that we have a separate Filter and AdaptiveFilter enums that are separately applied to the encoder, and the API does not communicate that the Filter setting is ignored if AdaptiveFilter::Adaptive is also set.

By contrast, the image crate exposes the Filter as a single enum already which lists all the individual filters plus Filter::Adaptive, which I think makes for a much nicer API.

There may be cases where adaptive filtering is not an option (e.g. in decoding or in per-row APIs), in which case we can keep that as a RowFilter or some such and ideally make that enum private.

@Shnatsel
Copy link
Contributor Author

I'm happy to try and apply this change unless somebody objects, since we have some semver-breaking API changes queued already.

@fintelia
Copy link
Contributor

I've been wanting to make this change. Would also be nice to make the enum non-exhaustive so other adaptive filter strategies could be added in the future.

Could you make the change is the same semver-breaking PR? That way we can look at the entire new API design in one place.

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

No branches or pull requests

2 participants