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.

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

Add a few text-related generators #56

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

voidus
Copy link
Contributor

@voidus voidus commented Jun 4, 2023

Based on my fumbling at #52 I added this to my project and thought if might be something you'd want to include.

The code points are taken from Hedgehog. I did consider adjusting the weights of unicode to skew more towards printable characters, but theres a lot of codepoints in unicde and I thought it's better to have hex escape codes than being biased too much towards ascii or sth.

I'm not sure what an appropriate level of testing for code like this would be. I'd be happy to add some, but I think I need a little guidance on what to test.

This PR builds on #54 , if you're interested in merging this one but are concerned about chooseAny, let me know and I'll remove the dependency

@edsko
Copy link
Collaborator

edsko commented Jun 5, 2023

This all looks reasonable enough to me. Some suggestions:

  • Rather than making this part of the standard generators exported by .Generator, we make this a dedicated module; probably we don't want to pollute the general namespace with all kinds of text related generators (I suspect over time more might be added)
  • I have no problem with the dependency on Add chooseAny, a version of choose that takes NonEmpty (Gen a) #54 (which I've merged but renamed chooseAny to oneof), but let's avoid the nix stuff if we can (there are some flake files added in this PR).
  • In terms of testing, I would let say, let's add a few tests to TestSuite/Prop/Generator, maybe a few shrinking tests or minimal value tests for some of these generators. Having a few checks in there will already prevent a lot of regressions, doesn't have to be anything particularly fancy.

@edsko
Copy link
Collaborator

edsko commented Jun 5, 2023

Thanks for the contribution!

@edsko
Copy link
Collaborator

edsko commented Jun 5, 2023

(I do like the "shrink towards printable" goal you mentioned in #52; that is something we can write a test for.)

@voidus
Copy link
Contributor Author

voidus commented Jun 7, 2023

rebased out the accidentally commited flake.{nix,lock}, I'll look into the other comments when I find time

@voidus voidus force-pushed the text branch 2 times, most recently from 75a524e to 515f8de Compare July 16, 2023 16:33
@voidus
Copy link
Contributor Author

voidus commented Jul 16, 2023

I reworked stuff a little and added tests. (They found issues of course 😃)

Still need to have a short think of how to best test string and text, will look into that later.

@voidus voidus force-pushed the text branch 3 times, most recently from edc7c8a to 1c71cf2 Compare July 16, 2023 22:57
@edsko
Copy link
Collaborator

edsko commented Nov 8, 2023

Marked this as WIP. Feel free to ping me when you think this is ready for review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants