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

Run mypy on tests #43

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Run mypy on tests #43

merged 7 commits into from
Aug 14, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Aug 13, 2024

This allows us to use Pydantic's model APIs directly, instead of going through a TypeAdapter.

This changeset also enables typechecking on our unit tests.

This allows us to use Pydantic's model APIs directly, instead
of going through a `TypeAdapter`.

This changeset also enables typechecking on our unit tests.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw requested a review from DarkaMaul August 13, 2024 18:26
@woodruffw woodruffw self-assigned this Aug 13, 2024
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@DarkaMaul
Copy link
Collaborator

Are we sure we want to follow this road?

from pydantic import Field, TypeAdapter, RootModel
from typing import Annotated
from pypi_attestations import GitHubPublisher, GitLabPublisher

gh_raw = {"kind": "GitHub", "repository": "foo/bar", "workflow": "publish.yml"}
P_before = Annotated[GitHubPublisher | GitLabPublisher, Field(discriminator="kind")]
P_after = RootModel[Annotated[GitHubPublisher | GitLabPublisher, Field(discriminator='kind')]]

assert isinstance(TypeAdapter(P_before).validate_python(gh_raw), GitHubPublisher) # True
assert isinstance(P_after.model_validate(gh_raw), GitHubPublisher) # False

IMO, using TypeAdapter once is better than having to deal with root all the time.

test/test_impl.py Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member Author

IMO, using TypeAdapter once is better than having to deal with root all the time.

Yeah, I have mixed feelings about it -- on one hand IMO it makes the types more uniform here, but the need for .root is definitely an eyesore.

There are other beneficial changes in this PR, so I'll remove the RootModel bits for now and open an issue for discussion.

woodruffw and others added 3 commits August 14, 2024 11:20
Co-authored-by: dm <alexis.challande@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw changed the title impl: make Publisher a RootModel Run mypy on tests Aug 14, 2024
@woodruffw woodruffw merged commit b290811 into main Aug 14, 2024
4 checks passed
@woodruffw woodruffw deleted the ww/better-types branch August 14, 2024 21:10
Copy link
Collaborator

@DarkaMaul DarkaMaul left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants