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

in_toto_attestation/v1: fix type hints #301

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

woodruffw
Copy link
Contributor

Fixes #300.

My editor made some formatting changes as well, based on the black rules -- let me know if you'd prefer to not have these, and I'll limit this PR to just the type fix.

@woodruffw woodruffw requested a review from a team as a code owner December 7, 2023 22:14
Comment on lines -22 to +33
def copy_from_pb(proto: type[rdpb.ResourceDescriptor]) -> 'ResourceDescriptor':
def copy_from_pb(proto: rdpb.ResourceDescriptor) -> "ResourceDescriptor":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NB: I thought a bit about this, and I think this hint was wrong to begin with: type[T] is used an annotation for class objects themselves, which isn't what this API takes.

Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

Aside: IMHO we should standardise on black code style & lint Python changes with black in the CI.

@woodruffw
Copy link
Contributor Author

Aside: IMHO we should standardise on black code style & lint Python changes with black in the CI.

Sounds good, I can do this 🙂

(Unless there's a very strong preference, I like ruff format -- it's style compatible with black, but about 100x faster.)

@adityasaky
Copy link
Member

would we set it up in CI to ignore the proto generated files?

@woodruffw
Copy link
Contributor Author

would we set it up in CI to ignore the proto generated files?

That would work, or you can have the CI format all files unconditionally -- as long as the formatting is part of the codegen step and is deterministic/has a fixpoint, I don't think it matters much (and has the added benefit of making it a little easier to read the generated code).

@adityasaky
Copy link
Member

part of the codegen

makes sense! That's currently invoked in CI here: https://github.com/in-toto/attestation/blob/main/.github/workflows/make-protos.yml. Adding the formatting to the Makefile for the python target should be sufficient.

@woodruffw
Copy link
Contributor Author

Does someone have merge permissions here? I'd like to land this in a release to unblock the work on DSSE support in sigstore-python 🙂

@TomHennen TomHennen merged commit 325dc92 into in-toto:main Dec 11, 2023
1 check passed
@TomHennen
Copy link
Contributor

Does someone have merge permissions here? I'd like to land this in a release to unblock the work on DSSE support in sigstore-python 🙂

Done. I'm never sure what the right etiquette here is.

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.

in_toto_attestation broken on Python < 3.9
4 participants