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

Hide modules that don't need to be publicly exported #548

Merged
merged 1 commit into from Jul 12, 2018

Conversation

sicking
Copy link
Contributor

@sicking sicking commented Jul 12, 2018

We're currently publicly exporting a lot of sub-modules under rand::distributions, even though we re-export the actual distribution directly under rand::distributions. So for example the Gamma distribution is exported both as rand::distributions::Gamma and rand::distributions::gamma::Gamma. We hide the latter from the documentation, but it's still there and could still be relied upon by code.

(Somehow I managed to repeat this mistake for the Weighted distribution, even though @dhardy pointed out that it wasn't needed. I thought I had fixed that but apparently not).

This PR fixes that, which also means that we can remove a bunch of #[doc(hidden)] and #[doc(inline)].

We also have two helper functions that are used by various distributions that currently live in somewhat awkward places. The ziggurat function lives directly in distributions/mod.rs, whereas log_gamma has its own file in distributions/log_gamma.rs. Neither of these functions are publicly exported, but would still be nice to move them to a better place. Mainly the ziggurat function felt very awkward to have in distributions/mod.rs, and felt like it more belonged together with log_gamma somewhere. I moved them both to distributions/utils.rs, but I would also be happy to move them to a new file.

@dhardy
Copy link
Member

dhardy commented Jul 12, 2018

Good idea, but we should probably wait until after the 0.6 release. These are maintained for deprecation warnings only. See #507.

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

This looks different from #507 to me. I didn't touch anything that was related to the deprecated module since I wasn't sure what was going on there. This PR only touches things that live under distributions and isn't marked as deprecated in any way.

But given that I don't really understand #507 yet, I might be mistaken?

@dhardy
Copy link
Member

dhardy commented Jul 12, 2018

The original point in these #[doc(hidden)] things was to rename without breakage for existing users.

Since we were originally just aliasing the new names, we couldn't use #[deprecated(..)]. The point of the deprecated module is to create wrappers around the original items which function the same but have deprecation warnings. Good idea though it is rather tedious to do.

So eventually all of that can be deleted, but since this deprecation stuff hasn't even been published in a release yet, I think we should hold off for now.

I'm surprised there's no overlap with #507 actually; @pitdicker you didn't bother making wrappers for the distribution modules? I guess it doesn't matter too much.

@pitdicker
Copy link
Contributor

I'm surprised there's no overlap with #507 actually; @pitdicker you didn't bother making wrappers for the distribution modules?

Oops, missed those. I am all for no longer exporting those modules. Should we have deprecation warnings here too? I think those are imported through a module much more rarely (if ever) than the ones I added in deprecated.rs.

@dhardy
Copy link
Member

dhardy commented Jul 12, 2018

I doubt there was much usage of these sub-modules. I tried searching GitHub for "rand::distributions::normal" and it picks up some related stuff, but no direct hits (though it's difficult to tell).

So I think it's okay just to merge this PR actually.

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

If I understand correctly, there would be no point in simply waiting to land this PR after 0.6. I.e. we'd still have the concern that we're potentially breaking code without having had a deprecation warning for said code first.

So if I understand correctly, the action we could take is to do the same thing with the types affected here as we've done elsewhere and make sure that 0.6 gives deprecation warnings if these types are used.

I don't have a strong preference on if we should do that or not. Though it seems like it would require a lot of code churn since the names that we want to give deprecation warnings overlap with the non-exposed internal names. So the only way I can see doing it is to move the existing code to new names and install forwarding types which give deprecation warnings.

It also looks like none of the code from 0.3 or 0.4 works for these types. So we're only talking about providing deprecation warnings for people that are on 0.5?

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

Ah, didn't see @dhardy last comment before submitting previous comment.

@dhardy
Copy link
Member

dhardy commented Jul 12, 2018

Yes, lets not bother.

Merge this first or #477? Do they have much overlap?

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

I'm more excited to get #477 in, so lets do that one first. Don't think the overlap is bad though.

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

FWIW, there does seem to be a non-zero set of users that will break. But it looks like a lot of these are copies of the same code.

https://github.com/search?l=Rust&q=%22rand%3A%3Adistributions%3A%3Agamma%3A%3AGamma%22&type=Code

@vks
Copy link
Collaborator

vks commented Jul 12, 2018

@sicking

FWIW, there does seem to be a non-zero set of users that will break. But it looks like a lot of these are copies of the same code.

All of those seem to be Firefox forks, which seems to vendor Rand including the benchmark. I doubt this will result in breakage for anyone.

@sicking sicking mentioned this pull request Jul 12, 2018
…::distributions. Also move a couple of distribution helper functions into utils.rs.
@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

Pushed a merged version.

@sicking sicking mentioned this pull request Jul 12, 2018
@dhardy dhardy merged commit b9cc581 into rust-random:master Jul 12, 2018
@sicking sicking deleted the mods branch July 12, 2018 23:10
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.

None yet

4 participants