-
Notifications
You must be signed in to change notification settings - Fork 33
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
Have NamedValue.name be a Cow to allow for owned Strings #46
base: master
Are you sure you want to change the base?
Conversation
95dd586
to
49faa03
Compare
You can't have default types for generic structs, one possibility could have been something like this pub struct NamedValue<T, S> {
/// Reference to the field name the value belongs to
pub name: S,
/// The value
pub value: T,
}
pub type NamedValueOwned<T> = NamedValue<T, String>;
pub type NamedValueRef<'a, T> = NamedValue<T, &'a str>; I'm not sure I understood why you need this change 🤔 |
I basically made my own version of the pub trait ReadableRecord: dbase::ReadableRecord {
/// Read the record from the provided data.
fn read_from_iter<'a, I>(fields: I) -> Result<Self, FieldIOError>
where
I: Iterator<Item = Result<NamedValue<'a, FieldValue>, FieldIOError>>,
Self: Sized;
} I did it that way because then I can (A) implement this for our own implementation and (B) easily implement it for anything that can implement dbase::ReadableRecord. Because the iterator that yields the NamedValue for own implementation reads and decodes the strings from the DBF file as it's iterating, I need to store these owned Strings somewhere. At the moment I guess I could just make my own NamedValue struct but then making sure it works with the dbase::ReadableRecord trait is messy again since I'd need to convert them. I was also considering opening a PR to just change dbase::ReadableRecord like I did it above, but that would require bigger changes. |
To be honest I am a bit hesitant about this, its a small change but I liked using I'll think more about this
Since the names of fields in the file are written once in the header, does that mean you read the names each time you read a record / field ?
I think my idea when making the |
At the moment yes, and while I wrote that, the downsides of that also occurred to me. The thing is, since we are reading live from a file which is also written to be other processes, that could potentially alter the fields, there's not really a good way of caching the fields. That's also one reason of why we still use the dbase crate for reading some files, since that works much better for reading in a lot of records. Our solution is primarily used for occasionally accessing single records.
That works fine, but since FieldIterator is specific to the dbase crate, it can't really be used for other implementations, which is fine I think, but that's why I made my own subtrait with a generic iterator. |
This changes the type of
NamedValue
'sname
field to be aCow<str>
instead of&str
.The reason I need this change is because in the project we are using this crate in, we have an alternative DBF implementation that reads/writes files in-place.
For de-serializing records we want to support the same mechanism already provided by this crate (since we use this crate and our own, which doesn't implement everything, at the same time),.
But since field names are read and decoded just as the iterator that generates the
NamedValue
s in our case, we need to store the owned value inNamedValue
.I tried using a generic argument for
NamedValue
to have this backwards compaible, but Rust complains about the lifetime not being bound if I set a default value for the type parameter (but not if I don't...?). This was my original idea, which would have been backwards compatible: