Add helper function and docs for loading pubkeys #120
Conversation
This is pretty hard to figure out from the existing docs. The helper is intended to "just work" in as many cases as possible. For that reason it uses the "many" variant (some key files in fact contain more than one key) and auto-detects the encoding (in some contexts the two formats are used interchangeably). The downsides of the helper are that it loses some information (which key failed to parse specifically, what the ascii armor headers were in the case of the ascii encoding), and also that it does not work with a reader but with a byte slice. But since key files are generally not very large I think this is an acceptable trade-off. People can always look at the source and make their own variant if they need it.
| /// | ||
| /// This is a convenience wrapper around [`SignedPublicKey::from_bytes_many()`] | ||
| /// and [`SignedPublicKey::from_armor_many()`]. | ||
| pub fn parse_public_keys(data: &[u8]) -> Result<Vec<SignedPublicKey>, crate::errors::Error> { |
There was a problem hiding this comment.
It might be more consistent API wise to move this to SignedPublicKey:from_many, not sure though, thoughts?
There was a problem hiding this comment.
At that point the different argument type (byte slice vs reader) becomes a bit untidy. Of course that can be solved but I frankly don't love Rust's buffering abstractions and prefer to avoid it if possible, and making this a "convenience wrapper" gives an excuse to just use a slice to peek at the first byte. This is not really an argument against moving it, just noting that moving it puts this back onto the "open questions / todo" list.
Making a single lonely function is a bit strange perhaps, and merging it into SignedPublicKey is perhaps the obvious thing to do. In fact I think I'll make that change (when I have time).
The main reason I made it a separate function is because of my frustration at how long it took me to understand how I should be loading keys. If you want to understand, read on, but please don't take this personally, I am describing just my annoyance here, but if I didn't appreciate the work that has gone into rPGP I wouldn't be here.
A lot of the code is parsing and (de)serialization. This code contains multiple layers which wrap each other and are named similarly, traits implemented using macros in other modules, etc. This makes it hard to know how a part works without understanding the whole. Part of this is because not just data but also logic is organized around the data model of the RFC, which users don't (want to) know in this much detail. A user knows they have a public key file but in fact that's not a PublicKey, it is a SignedPublicKey. And these from_bytes functions are too camouflaged for me, they look like every other bit of (de)serialization logic (which as I said is a lot). Is the "composed" module even supposed to be public? It is one of only three modules without a description in the top-level doc page, and its doc page describes it as "Internal key definitions". The only reason I put the function there is because of the existing docs there.
In contrast, parse_public_keys stands apart from implementation details and has a "verb"-ish name clearly related to what I want to do, instead of a generic sounding deserialization function on a "noun"-ish struct that a user may not even be sure is the right one. Basically, after the annoyance I went through I wanted to try to understand and address the problem in a deeper way than just adding an example, and this was the simplest way.
There was a problem hiding this comment.
Thanks for the detailed comments, some notes below
This makes it hard to know how a part works without understanding the whole. Part of this is because not just data but also logic is organized around the data model of the RFC, which users don't (want to) know in this much detail.
Yes this is intended for the current level that is implemented, as this was meant as a low level building block to allow folks who want to build their own tooling, or support their own use cases. I always intended to eventually have a high level, user friendly api that did not require reading the whole RFC before using it :D
Because of that it might make sense to actually take this method and start putting it into its own module, that can eventually be built out to provide such a higher level api.
Is the "composed" module even supposed to be public? It is one of only three modules without a description in the top-level doc page, and its doc page describes it as "Internal key definitions".
It is another low level building block, which certainly needs better documentation, and is mainly exposed currently in this form to make things usable, not because it is a well and nice defined API
Related to #44. Not closing yet as this PR only covers public keys.
This is pretty hard to figure out from the existing docs.
The helper is intended to "just work" in as many cases as possible. For
that reason it uses the "many" variant (some key files in fact contain
more than one key) and auto-detects the encoding (in some contexts the
two formats are used interchangeably).
The downsides of the helper are that it loses some information (which
key failed to parse specifically, what the ascii armor headers were in
the case of the ascii encoding), and also that it does not work with a
reader but with a byte slice. But since key files are generally not very
large I think this is an acceptable trade-off. People can always look at
the source and make their own variant if they need it.