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

An accessor for the whole headers, as sent #77

Closed
wants to merge 1 commit into from

Conversation

aredridel
Copy link
Contributor

@aredridel aredridel commented Jul 8, 2020

Reading the source, this naming makes sense — encoded meaning "as sent", raw meaning "before character set changes"; without looking at the source, I find it a bit inscrutable. I've matched the naming of get_body_encoded() when implementing get_heades_encoded() as mentioned in #75.

It might be worth clarifying some of this naming if there's a major version bump, but I hope this is acceptable as it is for now.

@cybernoid
Copy link

Thanks aredridel, that's pretty much identical to the code I wrote yesterday :-)

@cybernoid
Copy link

I'm not sure about the function name though. "header" most of the time refers to a single key/value pair, so I think it should be sth. like get_headers_encoded() or get_headerblock_encoded().

@aredridel
Copy link
Contributor Author

Whoops! Typo in my PR: I named it get_headers_encoded()

@staktrace
Copy link
Owner

My initial preference was to call this get_headers_raw but what you said about how the API uses encoded vs raw kind of makes sense too. I'm kind of on the fence about it. I agree that the way we ended up using these terms is pretty bad, and perhaps in the future with a major version bump I'll address that.

Alternatively, we could instead have this new API be a "split" type of API and return a tuple of (&[u8], &[u8]) so it can return both the header block and the body block which might be useful too. Thoughts on that?

@aredridel
Copy link
Contributor Author

What I'd really love is objects returned with a lifetime tied to the input, that themselves contain the ref, and have methods for further parsing — much like Body. If a Message is the input, plus methods that return the body and headers, and the headers have methods for returning individual headers, and the body has methods for individual nested parts, there's an elegant symmetry to it. Then the containing structs can have convenience accessors for common uses. Everything can be made pretty lazy, and everything is zero-copy by default. A split API that returns a Header and Body wrapper for those &[u8] sound the most delighful to me.

@staktrace
Copy link
Owner

Ah indeed, that does sound pretty nice. A Headers structure that exposes everything currently exposed by the .headers field and MailHeaderMap trait, plus the new "raw" header block.. I like it! And then the existing header APIs can be deprecated in favour of the new ones.

To start with we can just create the Headers structure and have it expose the raw byte data of the whole header block, which should be sufficient for #75. If you want to write that patch feel free, or I can work it later this week.

@cybernoid
Copy link

I like it as well, sounds like a clean approach and should not be too much work to implement!

staktrace added a commit that referenced this pull request Jul 10, 2020
staktrace added a commit that referenced this pull request Jul 10, 2020
@staktrace
Copy link
Owner

Obsoleted by 40c687b. Thanks all!

@staktrace staktrace closed this Jul 11, 2020
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

3 participants