Skip to content

Conversation

pitdicker
Copy link
Contributor

@pitdicker pitdicker commented Jun 9, 2018

We want to eventually remove the compatibility re-exports, but have no way to give deprecation warnings on those. As a hacky solution I added a bunch of newtypes.

It would be nice if crates that upgrade to rand 0.5 would directly get this warning and start using the new module organisation. @dhardy Sorry I didn't think of this yesterday, could you make another release?

Most deprecation messages will say "import with ... instead". For IsaacRng and Isaac64Rng the message also suggest to switch to Hc128Rng. For StdRng and ThreadRng it first recommends using the prelude.

jitter was a public, but hidden, module in the rngs module to make the comparability re-export easier. I just made that module private now, I can't imagine anyone depending on it already and it is almost impossible to give depreciation warnings for.

TimerError did not seem important enough to add a warning for, having it for JitterRng should suffice.

I optimistically set the release date on tomorrow. Does it after that make sense to merge the 0.5 branch back into master with these changes, and then to remove all the deprecated exports?

@pitdicker pitdicker force-pushed the deprecation_warnings branch 2 times, most recently from ea70eeb to 3614304 Compare June 9, 2018 16:11
@pitdicker
Copy link
Contributor Author

I have no idea why, but the CI for wasm-unknown-unknown failed with:

error: missing documentation for a method
   --> src/rngs/jitter.rs:748:5
    |
748 |     pub fn test_timer(&mut self) -> Result<u8, TimerError> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So I removed some of the #[cfg(..)] weirdness in JitterRng.

@pitdicker pitdicker mentioned this pull request Jun 9, 2018
@pitdicker pitdicker force-pushed the deprecation_warnings branch from 3614304 to dc16794 Compare June 9, 2018 18:26
@pitdicker
Copy link
Contributor Author

Updated to mention #505 in the changelog.

@pitdicker pitdicker force-pushed the deprecation_warnings branch from dc16794 to a0ea59e Compare June 9, 2018 18:37
@pitdicker pitdicker force-pushed the deprecation_warnings branch from a0ea59e to 70d8945 Compare June 9, 2018 18:38
@dhardy
Copy link
Member

dhardy commented Jun 9, 2018

Good idea but I don't see any hurry for this; it's fine if we only show the deprecations in 0.6 and remove them in 0.7.

I'm also not keen on adding deprecations in now, because it may cause more breakage for people who already upgraded to 0.5 (some projects require warnings to be fixed immediately).

@pitdicker
Copy link
Contributor Author

Yes, it is a tradeoff. In the end it will be less work for everyone to make the changes once now when updating to 0.5. There don't seem to be all that many crates yet which have updated, so I think this is still the right moment. But it will be a little annoying for the few crates that have already updated and now get this warning.

some projects require warnings to be fixed immediately

I also like to see Rand build without warnings, so I kind of agree. But Rust takes the position that choosing to fix or even deny warnings is a choice the crate makes, and their own problem if it means a little more work. I kind of agree.

@dhardy
Copy link
Member

dhardy commented Jun 12, 2018

So what happens now if JitterRng is used? Does it just fail with TimerError::CoarseTimer? Better if it failed with NoTimer and better yet if JitterRng isn't even available.


No, sorry, 0.5 has launched, and already has around 1000 downloads per day. There's no reason this can't wait until 0.6 (I don't think it even makes much more work).

@pitdicker
Copy link
Contributor Author

So what happens now if JitterRng is used?

It fails with NoTimer. It should get the same fix as #505 should get on master. At least it should be configured out of EntropyRng.

No, sorry, 0.5 has launched, and already has around 1000 downloads per day. There's no reason this can't wait until 0.6 (I don't think it even makes much more work).

I don't agree. There is no problem with adding deprecation warnings in a minor release. But no problem, I'll open another PR.

@dhardy
Copy link
Member

dhardy commented Jun 13, 2018

Thanks.

Actually, I agree that it can be reasonable to make deprecations in minor releases, but in this case we would be introducing a large number of deprecations without a very good reason.

Sounds like it's reasonable to merge the JitterRng fix in 0.5.

@pitdicker
Copy link
Contributor Author

pitdicker commented Jun 15, 2018

The JitterRng commit is now in #512.

@pitdicker pitdicker closed this Jun 15, 2018
@pitdicker pitdicker deleted the deprecation_warnings branch June 15, 2018 17:15
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.

2 participants