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

Rename Signature::verify to is_valid #16

Closed
joshlf opened this Issue Jan 15, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@joshlf
Copy link
Member

joshlf commented Jan 15, 2019

Signature::verify takes a public key and a signature and returns a boolean. @zarvox has pointed out that it's not 100% obvious to somebody coming across code like if sig.verify(&pub) { ... } which return value means that the signature is valid and which means that it's invalid.

In order to make it more obvious, we should rename this method to is_valid.

@yonson2

This comment has been minimized.

Copy link
Contributor

yonson2 commented Jan 17, 2019

@joshlf I've submitted a PR regarding this issue #17, the only problem I have with it is that the EcdsaSignature trait already implemented an is_valid method, I looked at the body of the function and ended up renaming it to has_content, I don't know if you are ok with that naming convention and I'm open to suggestions regarding what to do about it.

@categulario

This comment has been minimized.

Copy link

categulario commented Jan 17, 2019

@yonson2 maybe you can use the fully qualified syntax for choosing which implementation to call so you don't have to rename anything:

https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#fully-qualified-syntax-for-disambiguation-calling-methods-with-the-same-name

@joshlf

This comment has been minimized.

Copy link
Member Author

joshlf commented Jan 17, 2019

@yonson2 Ah good point about the name conflict. How does is_valid_format sound?

@yonson2

This comment has been minimized.

Copy link
Contributor

yonson2 commented Jan 18, 2019

@categulario thanks for that, I'll have it in mind for future occasions for sure but I think in this particular case its better for each trait to have a different method to prevent ambiguity. As for which name I like @joshlf's suggestion better than the one I had initially set so I've submitted that change.

@joshlf joshlf closed this in 1b717cd Jan 19, 2019

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