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 unnecessary num_traits re-export #95

Closed
wants to merge 1 commit into from

Conversation

WalterSmuts
Copy link
Contributor

Nothing in the external API requires the consumer to use num_traits directly. Removing this will decouple the rustfft major version from the the num_traits major version.

FftNum is defined in terms of the FromPrimitive and Signed traits in the num_traits crate however, so whenever they undergo a breaking change, rustfft will still require a major version change.

Since this is a possible breaking change for consumers that happen to consume num_traits via rustfft, this would require a major version bump.

Nothing in the external API requires the consumer to use num_traits
directly. Removing this will decouple the rustfft major version from
the the num_traits major version.

FftNum is defined in terms of the `FromPrimitive` and `Signed` traits in the
num_traits crate however, so whenever they undergo a breaking change,
rustfft will still require a major version change.

Since this is a possible breaking change for consumers that happen to
consume num_traits via rustfft, this would require a major version
bump.
@ejmahler
Copy link
Owner

ejmahler commented Oct 23, 2022

Looks like this failed because it needs the neon PR. No big deal.

I think this may be a holdover from before FftNum existed - long ago, the trait bounds were specified directly in terms of num_traits traits, and I ended up adding FftNum purely as a way to make the trait bounds on functions simpler.

I'm definitely down to simplify the library, so I'm open to this change. It's a breaking change, so it will have to wait until major changes are allowed again in 2024. I'm fine with leaving this open until then.

@HEnquist
Copy link
Contributor

HEnquist commented Oct 29, 2022

Somewhat related: rust-num/num#421
It bothers me a little that the num crates, that are very mature and stable, and used basically everywhere, are still at pre-1.0 versions.

@WalterSmuts
Copy link
Contributor Author

So I went ahead and copied rustfft and realfft's examples in easyfft. My thinking is that, while only exporting the required structs (Complex in this case), it's pretty useless if you can't use the related definitions. So even though it may result in less breaking changes, it's not worth it IMO.

@ejmahler
Copy link
Owner

Thanks for continuing to investigate, and thanks for making the recommendation even though it didn't work out!

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

Successfully merging this pull request may close these issues.

3 participants