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.

Sign up for GitHub

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

Jump to bottom

remove Random.gentype #31988

Closed
wants to merge 3 commits into from
Closed

remove Random.gentype #31988

wants to merge 3 commits into from

Conversation

tpapp
Copy link
Contributor

@tpapp tpapp commented May 10, 2019

Fixes #31968 by replacing Random.gentype with Base.eltype.

Also fix the overlooked issues in the discussion of #31787.

In addition, incidental clarifications and typo fixes.

In the Die example, define eltype from the beginning, instead of fixing it later, but mention what would happen if we didn't.

@rfourquet

tpapp added 3 commits May 10, 2019 08:39
@tpapp
fix typos, clarify what methods should be defined.
be4a77c 
This was missed in JuliaLang#31787.
@tpapp
remove example for Random.CloseOpen01{T}}
229bf3e 
it is just confusing at this stage.
@tpapp
Replace gentype with eltype.
7e32b98 
In the Die example, define eltype from the beginning, instead of
fixing it later, but mention what would happen if we didn't.

Incidental typo fixes and clarifications.
@rfourquet
Copy link
Member

Thanks But I think you should separate (in a different PR) the documentation improvement (which doesn't involve gentype) from deleting gentype. The docs can be merged without thinking twice. Deleting gentype should not involve documentation, and requires a decision which is not totally taken yet, and IMO should be accompanied with an update of the eltype docstring.

@tpapp
Copy link
Contributor Author

tpapp commented May 10, 2019

OK, will cherry-pick this one and make two new PRs.

@rfourquet rfourquet added domain:docs This change adds or pertains to documentation domain:randomness Random number generation and the Random stdlib labels May 10, 2019
@rfourquet
Copy link
Member

make two new PRs.

Heu, why not keep this one open and open only one other PR?

@tpapp
Copy link
Contributor Author

tpapp commented May 10, 2019

why not keep this one open and open only one other PR?

Did that, putting the trivial docs changes in #31990.

But my understanding is that if changes are requested there, then it will diverge from this one so I would need a new PR for the gentype part. Sorry, I am casual git user and maybe I am missing something.

@rfourquet
Copy link
Member

Indeed, the two PRs should be independant. What you can do here is to revert the documentation commits which are now included in that other PR.

@tpapp
Copy link
Contributor Author

tpapp commented May 10, 2019

OK, I will figure this out once the other PR is merged (I am assuming that can happen relatively quickly).

@rfourquet rfourquet changed the title Tp/remove random gentype remove Random.gentype May 11, 2019
rfourquet
rfourquet reviewed May 11, 2019
View reviewed changes
stdlib/Random/docs/src/index.md
@@ -89,7 +89,7 @@ The object returned by `Sampler` is then used to generate the random values. Whe
```julia
rand(rng, sampler)
```
for the particular sampler returned by `Sampler(rng, X, repetition)`
for the particular sampler returned by `Sampler(rng, X, repetition)`.
Copy link
Member

Choose a reason for hiding this comment

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

Oh, the period was in this commit :)

@tpapp tpapp mentioned this pull request May 12, 2019
Open
2 tasks
@tpapp
Copy link
Contributor Author

tpapp commented May 12, 2019

Closing in favor of #32008.

@tpapp tpapp closed this May 12, 2019
@tpapp tpapp deleted the tp/remove-Random-gentype branch May 12, 2019 12:39
@rfourquet
Copy link
Member

Closing in favor of #32008.

Did you close here because you didn't know how to update this PR with some commits removed?

@tpapp
Copy link
Contributor Author

tpapp commented May 12, 2019

I got in a hopeless tangle with git after merging, so yes. Sorry.

@rfourquet
Copy link
Member

No worries! In case this can help: assuming you have 2 branches locally, one corresponding to the current state ("hopeless tangle") of your PR, say pr, and a new one, say new, corresponding to the new PR you just opened (with the state you want), you can do:

$ git checkout pr
$ git reset --hard new # basically assigns the "value" of new to pr 
$ git push --force origin pr

@tpapp
Copy link
Contributor Author

tpapp commented May 13, 2019

Thanks, I will do this next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfourquet rfourquet rfourquet left review comments

Assignees
No one assigned
Labels
domain:docs This change adds or pertains to documentation domain:randomness Random number generation and the Random stdlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

the future of Random.gentype
2 participants
@tpapp @rfourquet