-
Notifications
You must be signed in to change notification settings - Fork 584
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
Exclude .arpa
tld names from domains()
strategy
#3572
Exclude .arpa
tld names from domains()
strategy
#3572
Conversation
Wow, this is even more of a rabbithole than I expected. In writing this comment I consulted the following sources:
Based on https://www.iana.org/domains/arpa I'm willing to say that it's "special-use enough for me" and suggest that we discard |
7dea288
to
f51bb5b
Compare
Fairnuff, I adjusted the PR accordingly — what says you now, @Zac-HD? Should we split this PR into two because it’s now technically two separate changes? Also, would you like me to squash all commits into a single one, or will you squash on merge? |
I think they're sufficiently related to keep as one PR, and whatever is easiest for you - I gave up on having a particularly clean git history years back, since it's just not viable when mentoring newbies 🙂 |
.arpa
tld from domains()
and add emails(domains=)
argument
@Zac-HD I could use a little hand-holding with testing: when I run
I just rebased on the latest Hypothesis |
OK, the problem is that def domains():
import hypothesis.provisional
return hypothesis.provisional.domains()
def emails(
*,
domains: st.SearchStrategy[str] = LazyStrategy(domains, (), {}),
) -> SearchStrategy[str]: and while it's not the easiest reading, I think that's the best solution we're going to get. |
e19cf82
to
47402d4
Compare
.arpa
tld from domains()
and add emails(domains=)
argument.arpa
tld names from domains()
strategy
47402d4
to
c2d863a
Compare
77a9702
to
36375d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jens!
Thank you for wrapping up the PR, @Zac-HD! I promise, one day I’ll actually finish one… |
Fixes #3567, closes #3582.
@Zac-HD this is a Draft PR for now because we need to discuss a few things:
arpa
:hypothesis/hypothesis-python/src/hypothesis/provisional.py
Line 44 in 600fc8a
arpa
from that list and inverse the logic when creating the list to draw from in the PR.Things to do for me: