search

BurntSushi / quickcheck

Automated property based testing for Rust (with shrinking).
The Unlicense
2.4k stars 149 forks source link

Ramp size #102

Closed SeanRBurton closed 8 years ago

SeanRBurton commented 9 years ago

It makes sense to start the size off small and gradually ramp it up. Making this change uncovered some bugs relating to numeric overflow and gen_range bounds which I have now fixed.

BurntSushi commented 8 years ago

This is a big change. Could you please describe in more detail what's going on?

SeanRBurton commented 8 years ago

This includes some stuff from my other pull requests. I'm feeling a little confused about the etiquette around making multiple pull requests. Do you have a particular preference or a workflow that you use personally?

BurntSushi commented 8 years ago

@SeanRBurton Here is my key concern: every pull request I accept means that I'm stating: yes, this is code I would like to maintain. In order to be OK with that, I have to understand what the code is doing and what problem it's solving. This PR contains quite a lot of changes, but a cryptic title and description that doesn't really help me understand the code, so it's hard for me to accept as is.

My personal preference is that you open issues discussing a big change like this first (similarly for auto-shrinking). Smallish PRs like your refactoring are totally fine to throw against the wall. I think the key goal of opening an issue first is: 1) making me understand the problem you're trying to solve and 2) convincing me that it's a problem worth solving. (I use "me" here because I'm the only maintainer of this project, but if there are more maintainers in the future, than "me" expands to include them as well.)

With all that said, it seems like this PR is meant to be fixing changes you made in #100? If so, that's a problem, because I didn't realize #100 needed fixing and I don't understand why it does now.

SeanRBurton commented 8 years ago

OK, this should be much better. Sorry about the earlier confusion. For the record, all fixed bugs were pre-existing.

BurntSushi commented 8 years ago

With the revert of #100, I think we need to go back to square 1 on this auto-shrinking stuff. It's my fault for not sticking to my guns and merging code that I didn't understand. If you want to help me work through it, I'd be happy to revisit.