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

feat: trait typing #818

Merged
merged 34 commits into from
Sep 11, 2023
Merged

Conversation

maartenbreddels
Copy link
Contributor

@maartenbreddels maartenbreddels commented Dec 20, 2022

Took #788 and vidartf#1 did some fixes for py < 3.10 and squashed it to make rebasing to master easier.

Closes #788

vidartf and others added 4 commits December 20, 2022 10:14
Co-authored-by: Steven Silvester <steven.silvester@ieee.org>
Co-authored-by: Vidar Tonaas Fauske <510760+vidartf@users.noreply.github.com>
This makes use of Self, which only seems to work with the current
mypy main branch.
@maartenbreddels
Copy link
Contributor Author

Failures are expected due to not being on master (where this would be green).

Also, would require a bit more work to get all the other types supported.

As @rmorshea said in #788 (comment) there is no way but to copy-paste the __new__ overloads and modify them for each trait. I have to say that even with more advanced typing, I don't see us avoid this, since each trait is a bit different.

@maartenbreddels maartenbreddels marked this pull request as draft December 20, 2022 15:53
@blink1073
Copy link
Contributor

I'd say partial support is a good start, we can follow up with the rest.

@maartenbreddels
Copy link
Contributor Author

I'd like to add support to a few more, and see if we can do without the Self and simply add some more copy-pasting.

@maartenbreddels
Copy link
Contributor Author

My guess is that the windows failures are solved by our good friend @vidartf who is always ahead of what we need for windows: davidfritzsche/pytest-mypy-testing#34
Is that correct @vidartf ? Does the error in CI look familiar to you?

@maartenbreddels
Copy link
Contributor Author

cc @rmorshea

Btw, I've moved away from overloading __new__, but we are now overloading __init__, by annotating self.

class Float(TraitType[G, S]):
    """A float trait."""

    default_value = 0.0
    info_text = "a float"

    @t.overload
    def __init__(
        self: "Float[float, t.Union[int, float]]",
        default_value: t.Union[float, Sentinel] = ...,
        allow_none: Literal[False] = ...,
        **kwargs: t.Any,
    ):
        ...

    @t.overload
    def __init__(
        self: "Float[t.Optional[int], t.Union[int, float, None]]",
        default_value: t.Union[float, Sentinel, None] = ...,
        allow_none: Literal[True] = ...,
        **kwargs: t.Any,
    ):
        ...

    def __init__(self, default_value=Undefined, allow_none=False, **kwargs):
        self.min = kwargs.pop("min", -float("inf"))
        self.max = kwargs.pop("max", float("inf"))
        super().__init__(default_value=default_value, allow_none=allow_none, **kwargs)

This looks way less scary than what we suggested in #788

@@ -25,7 +25,7 @@ class App(Application):
)

def start(self):
print(f"key={self.key}")
print(f"key={self.key.decode('utf8')}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typing is already paying off

@blink1073
Copy link
Contributor

Okay, so basically we need to wait for a new release of mypy, and then if there is not a release of davidfritzsche/pytest-mypy-testing#34, skip the typing tests on Windows.

@blink1073
Copy link
Contributor

mypy 1.0 has been released. It seems to me that pytest-mypy-testing isn't going to get a release. I would vote against depending on it, and instead have a set of files that we run mypy directly against.

@saulshanabrook
Copy link

Not to get too off-topic here, but @blink1073 I am also looking at using pytest-mypy-testing in a project but also am seeing that it is out of date.

I was thinking about trying instead to use MyPy's own unit testing framework, which is built on pytest, and was curious if you all had considered that as well.

I wasn't sure if anyone was using this outside of MyPy though.

@maartenbreddels
Copy link
Contributor Author

Interesting, I think it is tricky to get typing right without testing both successes and failures.

@saulshanabrook
Copy link

@maartenbreddels Yes I agree. I wasn't sure if you were implying that the mypy unit testing framework doesn't support testing failures as well, but here is a simple example of using it verify some things that pass and some that fail: https://github.com/python/mypy/blob/54635dec2379e2ac8b65b6ef07778015c69cfb6a/test-data/unit/check-basic.test#L93-L99

@saulshanabrook
Copy link

saulshanabrook commented Feb 24, 2023

I just had a go at trying to use MyPy's builtin testing framework in my package, and it seems to work OK! Only requires one monkeypatch...

egraphs-good/egglog-python#6

You can see the test file here which verifies that one way of calling a method passes MyPy and the other raises an error: https://github.com/metadsl/egg-smol-python/blob/db300cba4413f2b91cd0b8c3c9ae9cecab089ad1/test-data/unit/check-basic.test

@blink1073
Copy link
Contributor

I wonder if the mypy folks would support making the plugin public and not require the monkey-patch

blink1073

This comment was marked as resolved.

@blink1073 blink1073 marked this pull request as ready for review September 11, 2023 03:38
@blink1073
Copy link
Contributor

All green, let's gooo!

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

Successfully merging this pull request may close these issues.

5 participants