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

Exclude .arpa tld names from domains() strategy #3572

Merged

Conversation

jenstroeger
Copy link
Contributor

@jenstroeger jenstroeger commented Feb 5, 2023

Fixes #3567, closes #3582.

@Zac-HD this is a Draft PR for now because we need to discuss a few things:

  • turns out that the list of default top-level domain names did not contain special-use domain names, except arpa:
    TOP_LEVEL_DOMAINS = ["COM"] + sorted(_tlds[1:], key=len)
    Should I add them?
  • Alternatively we’d remove arpa from that list and inverse the logic when creating the list to draw from in the PR.
  • Hardwire the special-use domain names, or use a vendor file?
  • I wasn’t sure about your preference re import naming.

Things to do for me:

@Zac-HD
Copy link
Member

Zac-HD commented Feb 5, 2023

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 ARPA when loading tlds from our vendored file. I think then we can avoid adding the allow_special_use argument discussed in the issue, though adding a domains= argument to st.emails() would still be nice.

@jenstroeger jenstroeger force-pushed the add-special-use-domains-support branch from 7dea288 to f51bb5b Compare February 5, 2023 21:44
@jenstroeger
Copy link
Contributor Author

jenstroeger commented Feb 5, 2023

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?

@Zac-HD
Copy link
Member

Zac-HD commented Feb 8, 2023

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 🙂

@Zac-HD Zac-HD changed the title Add support for special-use domain names to domains() and emails() stratieg Exclude .arpa tld from domains() and add emails(domains=) argument Feb 10, 2023
@jenstroeger
Copy link
Contributor Author

@Zac-HD I could use a little hand-holding with testing: when I run make check-coverage as described here I get this errror:

coverage: commands[0]> python -bb -X dev -m coverage run --rcfile=.coveragerc --source=hypothesis -m pytest -n0 --ff tests/cover tests/conjecture tests/datetime tests/numpy tests/pandas tests/lark tests/redis tests/dpcontracts tests/codemods tests/typing_extensions
ERROR: while parsing the following warning configuration:

  ignore::hypothesis.errors.NonInteractiveExampleWarning

This error occurred:

Traceback (most recent call last):
  File "/.../hypothesis/hypothesis-python/.tox/coverage/lib/python3.10/site-packages/_pytest/config/__init__.py", line 1690, in parse_warning_filter
    category: Type[Warning] = _resolve_warning_category(category_)
  File "/.../hypothesis/hypothesis-python/.tox/coverage/lib/python3.10/site-packages/_pytest/config/__init__.py", line 1728, in _resolve_warning_category
    m = __import__(module, None, None, [klass])
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "/.../hypothesis/hypothesis-python/.tox/coverage/lib/python3.10/site-packages/_pytest/assertion/rewrite.py", line 168, in exec_module
    exec(co, module.__dict__)
...
  File "/.../hypothesis/hypothesis-python/.tox/coverage/lib/python3.10/site-packages/_pytest/assertion/rewrite.py", line 168, in exec_module
    exec(co, module.__dict__)
  File "/.../hypothesis/hypothesis-python/.tox/coverage/lib/python3.10/site-packages/hypothesis/provisional.py", line 147, in <module>
    st.lists(
AttributeError: partially initialized module 'hypothesis.strategies' has no attribute 'lists' (most likely due to a circular import)

I just rebased on the latest Hypothesis master branch. Is there anything I need to do/know to run tests for this PR?

@Zac-HD
Copy link
Member

Zac-HD commented Feb 14, 2023

OK, the problem is that hypothesis.provisional has a top-level import of hypothesis.strategies, and vice-versa... and the solution is annoyingly subtle - we can't practically modify .provisional, so we need to import it inside a function... even though we're using it as a default argument. Via the public API I'd use @st.composite, but with internals we can use LazyStrategy directly and save a tiny bit of overhead for subsequent draws:

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.

@jenstroeger jenstroeger force-pushed the add-special-use-domains-support branch 3 times, most recently from e19cf82 to 47402d4 Compare February 15, 2023 11:16
@jenstroeger jenstroeger changed the title Exclude .arpa tld from domains() and add emails(domains=) argument Exclude .arpa tld names from domains() strategy Feb 15, 2023
@Zac-HD Zac-HD force-pushed the add-special-use-domains-support branch from 47402d4 to c2d863a Compare March 16, 2023 05:23
@Zac-HD Zac-HD marked this pull request as ready for review March 16, 2023 05:24
@Zac-HD Zac-HD enabled auto-merge March 16, 2023 05:24
@Zac-HD Zac-HD force-pushed the add-special-use-domains-support branch 2 times, most recently from 77a9702 to 36375d5 Compare March 16, 2023 05:42
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jens!

@Zac-HD Zac-HD merged commit 7e6725b into HypothesisWorks:master Mar 16, 2023
@jenstroeger
Copy link
Contributor Author

Thank you for wrapping up the PR, @Zac-HD! I promise, one day I’ll actually finish one…

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

Successfully merging this pull request may close these issues.

Explicit support for special-use domains.
2 participants