-
Notifications
You must be signed in to change notification settings - Fork 1
Add function to get all header keys from Email object #2
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Matthias Beyer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! Adding such a function makes sense. I suggest we go with the following name (to match RFC terminology) and signature (to hide implementation details):
pub fn header_field_names(&self) -> Vec<&str>Also, could you please add a doc comment and test (see tests/test_fields.rs) for this?
Signed-off-by: Matthias Beyer <[email protected]>
|
If you're fine with the changes I'll fixup-rebase 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Besides a small test improvement (see inline comment), looks good.
| email.header_field_names() | ||
| .iter() | ||
| .all(|name| email.header_field(name).is_some()) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to check that what's returned by header_field_names is exhaustive (i.e., all field names present in the email are included).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the assertion do this? I mean, if one header_field_name is NOne when asking for header_field(header_field_name), that assert fails, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current assertion correctly checks that all names returned by header_field_names() are proper/present field names, but not that these names are the full list of names present in the email. For example, if the header contains "To", "Cc", "From" fields, but due to some bug header_field_names() returns only "To" and "Cc", this test will still pass.
This is helpful if the user of the
Emailobject does not know which headers exist.