search

BurntSushi / quickcheck

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

Document and expose qc_gen_size #197

Closed Centril closed 6 years ago

Centril commented 6 years ago

In this PR

Motivation

So that I can provide interoperability with quickcheck for proptest as seen here without copying the function.

PS: see https://github.com/AltSysrq/proptest/issues/18 for main discussion of the interoperability layer - it will either get added to proptest itself or to a separate crate.

Centril commented 6 years ago

[The promised crate]

BurntSushi commented 6 years ago

Sorry, but I don't understand this feature request. The first link in your issue doesn't work. I don't see any reason why we should just expose qc_gen_size. Could you please describe in more detail the problem you're trying to solve?

Centril commented 6 years ago

@BurntSushi Oh sorry, my bad - forgot to update the link. Here's the updated one: https://github.com/Centril/proptest-quickcheck-interop/blob/master/src/lib.rs#L149-L159

If qc_gen_size is not exposed I have to copy that bit, and if you ever change it, then I have to change it as well assuming I am notified of the change, which I might not be.

BurntSushi commented 6 years ago

@Centril I still don't understand why you need that specific helper function. What changes are you worried about? I don't think the environment variable name can change without a semver bump, for example, and I can say pretty confidently that the environment variable name probably shouldn't change at this point. The only other thing I can see if the default value, but I don't see that being a problem if they became out of sync for some reason.

In general, we shouldn't just add new things to the API. There should be a consistent story the motivates their exposure. For example, there are several other similar helper functions. Should they get exported too?

Centril commented 6 years ago

My only concern was the default value since the env-var was obviously semver constrained - but as you don't believe that's a problem, I'll go ahead and close the PR. =)