flyingmutant / rapid

Rapid is a modern Go property-based testing library
https://pkg.go.dev/pgregory.net/rapid
Mozilla Public License 2.0
579 stars 25 forks source link

Add generators for URLs and domain names #18

Open wfscheper opened 3 years ago

wfscheper commented 3 years ago

This adds an RFC 1035 compliant domain name generator and an RFC 3986 compliant URL generator.

Closes #17

Signed-off-by: Walter Scheper ratlaw@gmail.com

flyingmutant commented 3 years ago

Thanks a lot for the PR! I'll try to review in the next couple of days. Can you please add your copyright on top of the new files?

wfscheper commented 3 years ago

Will do. I also realized I should add Examples, so I'll push them as well.

flyingmutant commented 3 years ago

By the way, any idea how to make CI work for this PR? It seems stuck.

wfscheper commented 3 years ago

I don't think anything is being scheduled for the PR, but I'm not sure why.

Edit: just looked at the workflow file, and there's no trigger for pull_requests. I can add that if you want. Here's one of my projects for comparision: https://github.com/wfscheper/stentor/blob/main/.github/workflows/build.yml

flyingmutant commented 3 years ago

Shouldn't "on (any) push" be enough?

Also, can you please try rebasing on top of current master? Maybe it can help CI to unstuck itself as well.

wfscheper commented 3 years ago

I think GitHub treats pushes to pull requests differently than pushes to "normal" branches.

I'll try a rebase later today.

On Thu, Nov 12, 2020, 11:28 Gregory Petrosyan notifications@github.com wrote:

Shouldn't "on (any) push" https://github.com/flyingmutant/rapid/blob/master/.github/workflows/ci.yml#L3 be enough?

Also, can you please try rebasing on top of current master? Maybe it can help CI to unstuck itself as well.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flyingmutant/rapid/pull/18#issuecomment-726186571, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPTM5GSAMBL5HO2WDFC7TSPQEL3ANCNFSM4TGGHHMA .

flyingmutant commented 3 years ago

I've pushed the on pull_request change to the CI config, and created #20 to test it. Looks like CI change is working -- but the CI itself is failing on Go 1.13.

Can you please rebase this PR on top of current master and look into the CI failure? I think once the CI is happy I'll merge the PR, and fix the nitpicks (if I'll find something else to nitpick, that is) afterwards.

wfscheper commented 3 years ago

I'm at a loss on how to address the 1.13 issue. I think I may need to install go 1.13 and try stepping through the example with delve. I should have some time tonight.

wfscheper commented 3 years ago

Hey, apologies for leaving this hanging for so long. I see that you added some build tags to try and get around the example tests not lining up correctly. I also added a bash script to update the tld constant, which was out of date. Something to wire into a github action, perhaps?

flyingmutant commented 3 years ago

No problem, rapid is not changing too rapidly :-) I use build tags mainly to work around Unicode database updating from release to release.

Something to wire into a github action, perhaps?

I think just doing manual update every couple of releases might be good enough for now. I don't want to complicate the automation too much.

wfscheper commented 1 year ago

Hello! GitHub randomly reminded me about this abandoned PR of mine. Thought I'd take a bit and update it, and was pleasantly surprised by the new API using generics. Awesome stuff!

Only hiccup I had was the loss of the old Generator.Map() functionality. Is there an equivalent that I missed?

flyingmutant commented 1 year ago

Thought I'd take a bit and update it, and was pleasantly surprised by the new API using generics. Awesome stuff!

Yep, I was pleasantly surprised how little changes (conceptual, not LoC) were required to make rapid fully type-safe.

Only hiccup I had was the loss of the old Generator.Map() functionality. Is there an equivalent that I missed?

Methods can't have their own generic parameters, because of that Generator.Map() became top-level Transform.