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

add implementation for rand traits on dimensioned types #45

Merged
merged 3 commits into from Aug 14, 2018

Conversation

droundy
Copy link
Contributor

@droundy droundy commented Jul 31, 2018

This will allow users to directly create random values with dimensions, which seems like a significant win.

Copy link
Owner

@paholg paholg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! It's a shame rand_core doesn't have the appropriate traits, as then this could be added without the feature flag.

Looks like there are a couple conflicts with the ClapMe PR that need to be resolved, and if you don't mind, could you add this to the CHANGELOG.md? (I should really add a contributing guide that requests this).

/// This is an internal implementation detail that the rand
/// crate requires us to expose. Sorry about that.
#[derive(Clone, Copy, Debug)]
pub struct MyUniformSampler<V: SampleUniform, U> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an implementation detail, let's add #[doc(hidden)]

src/build/mod.rs Outdated
"
}}"
)?;
// done with serde test
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably read done with rand test

@paholg
Copy link
Owner

paholg commented Aug 12, 2018

Also, @droundy, by a request, I would like to bump the version and deploy dimensioned to crates today (as I am unlikely to have time this week).

If you can get this mergeable sometime today, it could join that deploy. Otherwise, we can also bump the version again.

I also added a tests-feature-combos.sh script in tests/ which I used
to find out where I broke other features.  Probably this should be
used in travis (it only took 3 minutes to run on my machine), but not
in a merge commit.
@droundy
Copy link
Contributor Author

droundy commented Aug 13, 2018

Hi @paholg, I've done the merge, and also added a little script to test feature combinations.

Copy link
Owner

@paholg paholg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@paholg paholg merged commit af1c51a into paholg:master Aug 14, 2018
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

2 participants