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 type hints to our public API (PEPs 484 & 561) #1270

Merged
merged 9 commits into from Jun 26, 2018

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented May 9, 2018

This PR finally closes #200 🎉

For anyone manually exploring this PR, note that Mypy does not find hints installed in editable mode (pip install -e .); you have to do a straight pip install . - see python/mypy#5007 for work on a fix. This does not affect our CI setup as we don't use editable mode there, but is annoying in local development.

@Zac-HD
Copy link
Member Author

Zac-HD commented May 22, 2018

🎉, I finally got the build working! The solution is to set the MYPYPATH rather than relying on a PEP 561 install thing, but I'm OK with that as any breakage would be hugely obvious upstream before these tests gave a false-pass.

CC @DRMacIver for review; it's mostly a matter of documentation and what counts as public API I think 😄

@Zac-HD
Copy link
Member Author

Zac-HD commented Jun 14, 2018

python/mypy#5218 is my report of (the first part I isolated of) the problem with one_of; later replaced by python/mypy#5269. I've noted and/or added a test for all the type inference issues I know of. Further work can wait for a later PR, I think!

Copy link
Contributor

@alexwlchan alexwlchan left a comment

Choose a reason for hiding this comment

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

I really don’t know much about type hinting or Mypy or the like. This looks plausibly correct, but I don’t feel I’m qualified to give this a green tick.


We may also find more precise ways to describe the type of various
interfaces, or change their type and runtime behaviour togther in a way
which is otherwise backwards-compatible. We often ommit type hints for
Copy link
Contributor

Choose a reason for hiding this comment

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

sp: “ommit”

env=dict(os.environ, MYPYPATH=tools.PYTHON_SRC),
).stdout.read()
assert len(out.splitlines()) == 1
typ = out.split('error: Revealed type is ')[1].strip().strip("'")
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what Mypy successful or unsuccessful output looks like – a brief comment here with examples of both would help satisfy me that this string parsing code is working correctly.

Copy link
Member Author

@Zac-HD Zac-HD Jun 16, 2018

Choose a reason for hiding this comment

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

I'll add a link to the docs and an example of the output we're parsing. (done)

@Zac-HD Zac-HD force-pushed the public-hints branch 5 times, most recently from 6d58963 to 16c0026 Compare June 25, 2018 03:23
Copy link
Member

@DRMacIver DRMacIver left a comment

Choose a reason for hiding this comment

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

One question about a thing I didn't understand, otherwise LGTM.

('booleans().filter(bool)', 'bool'),
('lists(none())', 'list[None]'),
('dictionaries(integers(), datetimes())', 'dict[int, datetime.datetime]'),
('recursive(integers(), lists)', 'Union[list[Ex`-1], int]'),
Copy link
Member

Choose a reason for hiding this comment

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

What is Ex`-1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ex is the type of the example which SearchStrategy is generic and covariant in; and Ex`-1 appears to denote recursion in this type - that is, Ex`-1 == Union[list[Ex`-2], int] and so on.
I'll add a comment to this effect...

Copy link

@ethanhs ethanhs left a comment

Choose a reason for hiding this comment

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

This looks sensible from a PEP 561 perspective (save the one suggestion). Also, mypy 0.620 should have editable install support FWIW.

@@ -86,7 +86,7 @@ def local_file(name):
author_email='david@drmaciver.com',
packages=setuptools.find_packages(SOURCE),
package_dir={'': SOURCE},
# package_data={'': ['py.typed']}, # un-comment to release type hints
package_data={'': ['py.typed']},
Copy link

Choose a reason for hiding this comment

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

I believe it is better to explicitly mark the package name this file is from.

As suggested by Ethan Smith
The test could only pass if no padding was generated, so adding pattern boundaries makes matching faster and exact generation less flaky.
@Zac-HD Zac-HD merged commit 8691d81 into HypothesisWorks:master Jun 26, 2018
@Zac-HD Zac-HD deleted the public-hints branch June 26, 2018 03:53
@Zac-HD
Copy link
Member Author

Zac-HD commented Jun 26, 2018

Finally! 🎉

@ethanhs
Copy link

ethanhs commented Jul 2, 2018

FWIW, support for editable installs just landed and will be in the next mypy release.

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.

Add static typing to Hypothesis API
4 participants