allow binary field values #13
Conversation
yu-re-ka
force-pushed
the
feature/binary-field-values
branch
from
a7ed90e to
eb262c6
Compare
| self.fields | ||
| .get(field) | ||
| .map(|v| str::from_utf8(v)) | ||
| .transpose() |
There was a problem hiding this comment.
This transpose is weird to me. It seems weird that I have to check for the Utf8Error before even knowing if the field exists.
| self.fields.get(field).map(|v| v.as_slice()) | ||
| } | ||
|
|
||
| pub fn get_field_string(&self, field: &str) -> Result<Option<&str>, str::Utf8Error> { |
There was a problem hiding this comment.
This should probably be get_field_str because it is a borrow.
| pub fn get_message(&self) -> Option<&str> { | ||
| self.fields.get("MESSAGE").map(|v| v.as_ref()) | ||
| pub fn get_message(&self) -> Option<Cow<'_, str>> { | ||
| self.get_field_string_lossy("MESSAGE") |
There was a problem hiding this comment.
I'm not a huge fan of being lossy by default but up to you. It is supposed to be convenient so i get the desire.
Maybe at least add a warning to the docs?
| self.get_field_string_lossy("_SOURCE_REALTIME_TIMESTAMP") | ||
| .and_then(|v| v.parse::<i64>().ok()) | ||
| .map(|v| JournalEntryTimestamp { timestamp_us: v }) |
There was a problem hiding this comment.
| self.get_field_string_lossy("_SOURCE_REALTIME_TIMESTAMP") | |
| .and_then(|v| v.parse::<i64>().ok()) | |
| .map(|v| JournalEntryTimestamp { timestamp_us: v }) | |
| self.get_field_string("_SOURCE_REALTIME_TIMESTAMP").ok() | |
| .flatten() | |
| .and_then(|v| v.parse::<i64>().ok()) | |
| .map(|v| JournalEntryTimestamp { timestamp_us: v }) |
Minor performance note, if a string isn't valid utf8 it isn't going to be parsable as in integer so no need to allocate the string for the lossy version. (and same with the other time functions below)
| let mut name_value = b.splitn(2, |x| *x == b'='); | ||
| let name = String::from_utf8_lossy(name_value.next().unwrap()).into_owned(); | ||
| let value = name_value.next().unwrap().to_vec(); | ||
| fields.insert(name, value); |
There was a problem hiding this comment.
I think only the from_raw_parts call is unsafe. It would be nice to move the rest out of the unsafe block to make it clear what the unsafe bit is.
| @@ -226,7 +225,7 @@ impl JournalReader { | |||
|
|
|||
| fields.insert( | |||
| "__CURSOR".to_string(), | |||
| cursor.to_string_lossy().into_owned(), | |||
| cursor.to_string_lossy().into_owned().as_bytes().to_vec(), | |||
There was a problem hiding this comment.
I don't think we need to go from CStr → String → Vec. It should be possible to go directly from CStr → Vec.
No description provided.