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

FillInReduction enum is not a good API for version stability #214

Closed
vbarrielle opened this issue Aug 25, 2020 · 3 comments
Closed

FillInReduction enum is not a good API for version stability #214

vbarrielle opened this issue Aug 25, 2020 · 3 comments
Labels
Milestone

Comments

@vbarrielle
Copy link
Collaborator

vbarrielle commented Aug 25, 2020

This enum lists all existing fill in reduction methods in the sprs ecosystem. I realized while binding such methods from suitesparse that each time a method would be added, this would require changing this enum and thus making a breaking change. This does not look like a good idea, so this enum should probably go away in the future.

@vbarrielle vbarrielle changed the title FillInReduction enum is not a good API for version stabilityt FillInReduction enum is not a good API for version stability Aug 25, 2020
@vbarrielle vbarrielle added this to the 0.9 milestone Aug 25, 2020
@mulimoen
Copy link
Collaborator

An alternative is to use #[non_exhaustive] on the affected enum

@vbarrielle
Copy link
Collaborator Author

Yes, I've been considering that as well. However I'm not sure if it's a good idea to expose this enum at all. It is used in LDL decomposition to specify decomposition options in a builder pattern (https://github.com/vbarrielle/sprs/blob/master/sprs-ldl/src/lib.rs#L132-L143), but there may be more idiomatic ways to do it (eg by passing a FnOnce that will perform the decomposition when the input matrix is passed). That way no need to match on all possible enums, and it's extendable with other crates as well.

@vbarrielle
Copy link
Collaborator Author

Fixed by #223, I've chosen to use #[non_exhaustive], passing a function would have made the API too cumbersome (right now one nice thing about the Ldl builder struct is that it does not depend on the actual type of the matrix up until the last step, and this property would have been lost).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants