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

Change more Random() methods to accept random sources (and some related tweaks) #2115

Merged
merged 2 commits into from
Feb 4, 2018
Merged

Change more Random() methods to accept random sources (and some related tweaks) #2115

merged 2 commits into from
Feb 4, 2018

Conversation

fingolfin
Copy link
Member

No description provided.

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Jan 25, 2018
ChrisJefferson
ChrisJefferson reviewed Jan 25, 2018
View reviewed changes
Copy link
Contributor

@ChrisJefferson ChrisJefferson left a comment

Choose a reason for hiding this comment

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

It would be nice to chuck some tests into testinstall/random.tst, to check that the changed random methods behave correctly.

@fingolfin
Copy link
Member Author

Agreed. So, I actually wanted to (a) get the full existing test suite running (and as we see, it already found failures ;-) and (b) was hoping that I could then use codecov to figure out which functions are already tested / which are not, and then provide tests for the missing ones (and see in codecov the results ;-).

@fingolfin fingolfin added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jan 29, 2018
@fingolfin fingolfin changed the title Change more Random() methods to accept random sources (and some related tweaks) WIP: Change more Random() methods to accept random sources (and some related tweaks) Jan 29, 2018
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.0 milestone Jan 29, 2018
@fingolfin fingolfin force-pushed the mh/random branch 2 times, most recently from 9eae2bb to cf03dd5 Compare January 31, 2018 15:00
@fingolfin fingolfin removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jan 31, 2018
@fingolfin fingolfin changed the title WIP: Change more Random() methods to accept random sources (and some related tweaks) Change more Random() methods to accept random sources (and some related tweaks) Jan 31, 2018
@fingolfin
Copy link
Member Author

I have now added tests for all changed places (and found and resolved a bug in my changes ;-). Sadly, codecov is broken for us and thus does not currently post PR comments (already told them about it, apparently, some commits in our history confuse codecov ?!? they are looking into it).

Anyway, you can see the coverage for this PR at https://codecov.io/gh/gap-system/gap/pull/2115/diff (but right now the PR is of course rebuilding, so the coverage report will be updated afterwards again ;-)

@fingolfin
Copy link
Member Author

Codecov PR comments don't work, but the coverage for this PR is 100%, as you can see on https://codecov.io/gh/gap-system/gap/pull/2115

@fingolfin fingolfin merged commit 29f0678 into gap-system:master Feb 4, 2018
@fingolfin fingolfin deleted the mh/random branch February 4, 2018 21:32
@olexandr-konovalov
Copy link
Member

@fingolfin is this for release notes for GAP 4.10? If so, what about a short phrase in the description, and maybe a better title (without "tweaks")?

@fingolfin fingolfin mentioned this pull request Feb 22, 2018
Open
18 tasks
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisJefferson ChrisJefferson ChrisJefferson approved these changes

@markuspf markuspf Awaiting requested review from markuspf

Assignees
No one assigned
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Milestone
GAP 4.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fingolfin @olexandr-konovalov @ChrisJefferson