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

Changing VerificationContent interface method to return certificate by pointer #75

Closed
haydentherapper opened this issue Jan 9, 2024 · 1 comment · Fixed by #209
Closed
Labels
enhancement New feature or request v1.0 items we want to consider for a v1.0 release

Comments

@haydentherapper
Copy link
Contributor

Description

HasCertificate returns a certificate by value along with a boolean to denote if the certificate was returned. A proposed simplification of this interface would be to change it to Certificate() (*x509.Certificate), which has two benefits: Passing by pointer over copy to avoid copying potentially large certificates, and removing the need to return a boolean when you can simply check if the return value is not nil.

One question for the original implementers: Was this function named to mirror the other function that returns an interface and boolean, since we can't return an interface by pointer?

@haydentherapper haydentherapper added the enhancement New feature or request label Jan 9, 2024
@steiza
Copy link
Member

steiza commented Jan 10, 2024

One question for the original implementers: Was this function named to mirror the other function that returns an interface and boolean, since we can't return an interface by pointer?

I think so, along with things like HasInclusionPromise() and HasInclusionProof() where we just return a bool. But I don't think we depend on this naming / consistency, and this proposed change sound fine to me.

@haydentherapper haydentherapper added the v1.0 items we want to consider for a v1.0 release label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v1.0 items we want to consider for a v1.0 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants