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

feat: allow encoding data without owning the it #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ctron
Copy link

@ctron ctron commented Jan 20, 2023

My use case is to encode existing data into the PEM format.

However, with the current API, I need to own the data, and so I do need to copy the arrays, just for encoding it. Which also doesn't seem to consume the data.

I added a new struct RefPem, which is intended to hold Pem data, just by reference. And modify the existing encode functions (not yet the "many" variants) to support using anything which can Into<RefPem>, which is implemented for &Pem too. So the API for the user shouldn't be different.

Copy link
Owner

@jcreekmore jcreekmore left a comment

Choose a reason for hiding this comment

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

After looking through this, I think it might be simpler just to introduce a couple of functions like encode_parts(tag: &str, contents: &[u8]) -> String and encode_parts_config(tag: &str, contents: &[u8], config: EncodeConfig) -> String and allow the user to just pass the pieces to those, leaving the existing functions as just a wrapper that extracts the tag and contents by reference from the Pem struct and passes them along.

use parser::{parse_captures, parse_captures_iter, Captures};
use std::borrow::Borrow;
Copy link
Owner

Choose a reason for hiding this comment

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

cargo build and cargo test throw unused import warnings about this.

contents: &[1, 2, 3, 4],
};

let encoded = encode_ref(pem.clone());
Copy link
Owner

Choose a reason for hiding this comment

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

encode_ref does not exist (and shouldn't given the approach of making this be transparent to the user) so I think you meant encode here.

Comment on lines +320 to +326
/// use pem::{RefPem, encode};
///
/// let pem = RefPem {
/// tag: "FOO",
/// contents: &[1, 2, 3, 4],
/// };
/// encode(&pem);
Copy link
Owner

Choose a reason for hiding this comment

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

This example fails because you don't have an Into<RefPem<'p>> implementation for a &RefPem<'p> itself. So does the other example you added for the encode_config function. That said, I am not sure that having an implementation to go from a &RefPem to a RefPem makes a whole lot of sense and it is going to be weird that one of the ways you call it is encode(&pem) and the other way is encode(ref_pem).

@ctron
Copy link
Author

ctron commented Jan 25, 2023

After looking through this, I think it might be simpler just to introduce a couple of functions like encode_parts(tag: &str, contents: &[u8]) -> String and encode_parts_config(tag: &str, contents: &[u8], config: EncodeConfig) -> String and allow the user to just pass the pieces to those, leaving the existing functions as just a wrapper that extracts the tag and contents by reference from the Pem struct and passes them along.

Sure this would work as well. Do you want me to change the PR?

@jcreekmore
Copy link
Owner

After looking through this, I think it might be simpler just to introduce a couple of functions like encode_parts(tag: &str, contents: &[u8]) -> String and encode_parts_config(tag: &str, contents: &[u8], config: EncodeConfig) -> String and allow the user to just pass the pieces to those, leaving the existing functions as just a wrapper that extracts the tag and contents by reference from the Pem struct and passes them along.

Sure this would work as well. Do you want me to change the PR?

Yeah. Otherwise, I won't have a chance to do it for a bit.

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.

None yet

2 participants