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

Making the public API more familiar #106

Closed
brycx opened this issue Oct 19, 2019 · 6 comments
Closed

Making the public API more familiar #106

brycx opened this issue Oct 19, 2019 · 6 comments
Labels
breaking change A breaking change question Further information is requested usability Issues or improvements related to UX

Comments

@brycx
Copy link
Member

brycx commented Oct 19, 2019

The public API for streaming structs and newtypes could be made more familiar to users that come from other Rust crypto libraries.

In continuation of trying to make the streaming API more consistent (#87) the general approach seems to be that all one-shot functions are also part of the struct (an example of this is the RustCrypto Digest trait). Maybe the current should be changed to:

  • module::one_shot_function() -> module::Ctx::one_shot_function()
  • module::verify() -> module::Ctx::verify()

For the newtypes API, to be more consitent with types throughout Rust, we chould change:

  • get_length -> len

Changing the newtype API seems worthwhile, but it's debatable whether changing the straming struct API will make a noticable difference in usability.

@brycx brycx added question Further information is requested usability Issues or improvements related to UX breaking change A breaking change labels Oct 19, 2019
@colelawrence
Copy link
Contributor

colelawrence commented Oct 19, 2019

Is moving the functions from module to impl potentially more ergonomic, because you can now call ctx.verify etc? And it will be based on the type of ctx itself?

@brycx
Copy link
Member Author

brycx commented Oct 19, 2019

It will be based on the implementation of Ctx itself. Currently the module has the same name as Ctx, but this might change in the future if more variants of Ctx are added to the module, for example both SHA256 and SHA512 exist in a sha2 module, thus making a module-level verify() function confusing and will result in duplicate version of it at module-level.

ctx.verify will not exactly be available though, because verifying using incremental processing isn't provided, so it'll remain that you need to provide all the data at call: Ctx::verify().

@colelawrence
Copy link
Contributor

Just for more information: we could technically add the new function and use a deprecated warning on the previous usage, right?

@brycx
Copy link
Member Author

brycx commented Oct 21, 2019

We could do that, but I don't think it makes much sense in this case. There is already a list of breaking changes coming up for the next major version bump. Marking these functions deprecated in the next major version would simply require another major version bump before they could be completely removed.

Aside from that, I personally don't feel marking functions as deprecated really does any good. For example, orion will fail to compile if any warnings are issued by the compiler, which will happen if one of its dependencies deprecates something (this happened with getrandom recently) and thereby break orion. So it'll depend on users whether deprecation is breaking or not.

@colelawrence
Copy link
Contributor

Gotcha! The approach sounds good to me.

@brycx
Copy link
Member Author

brycx commented Oct 27, 2019

Fixed in #107.

@brycx brycx closed this as completed Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A breaking change question Further information is requested usability Issues or improvements related to UX
Projects
None yet
Development

No branches or pull requests

2 participants