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 approx and rand from default features #6

Closed
2 tasks done
bitshifter opened this issue Jul 20, 2019 · 2 comments
Closed
2 tasks done

Remove approx and rand from default features #6

bitshifter opened this issue Jul 20, 2019 · 2 comments
Labels
enhancement New feature or request

Comments

@bitshifter
Copy link
Owner

bitshifter commented Jul 20, 2019

approx and rand define traits which must be implemented inside glam for them to be used in test and bench executables. Unfortunately this means they must be enabled by default so that a simple cargo test or cargo bench will work without needing to specify additional features. I'd rather not enable them by default.

For the purposes of glam's tests and benches it is probably easier to implement a simple non-crypto rand and some approx eq functions.

glam could use it's own approx_eq functions anyway as we don't re-export the approx crate, I'd prefer not to need users to have to import traits for this and the approx traits aren't documented much.

The rand crate is quite complex and we're not concerned with crypto strength random numbers for the purposes of our unit tests.

The current implementations of approx and rand can be kept unless it becomes a burden to maintain them.

  • approx
  • rand
@bitshifter bitshifter changed the title Remove approx and rand from default featuers Remove approx and rand from default features Jul 21, 2019
@bitshifter bitshifter added the enhancement New feature or request label Jul 22, 2019
@cessen
Copy link
Contributor

cessen commented Jul 23, 2019

I responded to your comment on my commit to Psychopath already, but reading this issue I realized I misunderstood a bit.

I think you don't actually need to have these enabled by default for using them in testing. You can just have them be a dev dependency, and have the impls hidden behind a #[cfg(test)] attribute. Maybe there are corner-cases I'm not aware of, but I think that works.

If that doesn't work, then rolling your own and making them testing-only definitely would, and I'd be in favor of that. I don't actually need approximate comparisons implemented in glam--I'm happy to roll my own as needed, and it seems orthogonal to glam's purpose.

@repi
Copy link
Contributor

repi commented Sep 14, 2019

Would also like to see both of them disabled by default, and ideally replacing approx::assert_ulps_eq in the tests with something internal in the crate to avoid always pulling in approx.

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

No branches or pull requests

3 participants