-
Notifications
You must be signed in to change notification settings - Fork 19
Add Writeable getters for some types, use in FFI #340
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
Conversation
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.
Small nit. The other is more of a general thought; I'll leave the action up to you.
@@ -91,6 +91,16 @@ impl<'a> IxdtfStringBuilder<'a> { | |||
} | |||
} | |||
|
|||
impl<'a> Writeable for IxdtfStringBuilder<'a> { |
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.
thought (non-blocking): this should be moved to an ixdtf
module or some other module.
The initial thought was to try and move this functionality up into ixdtf
. Still not entirely sure whether that should be done, but I don't think it should live in the parsers
mod. I think use temporal_rs::ixdtf::IxdtfStringBuilder
makes more sense than use temporal_rs::parsers::IxdtfStringBuilder
.
This can be done in a separate PR though. Also, open to other suggestions on where IxdtfStringBuilder
should live.
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.
I think this makes a lot of sense to live in the ixdtf
crate. A module also seems fine.
77382b4
to
258bbdf
Compare
I filled in the rest of the types. I couldn't do Duration because it doesn't use IxdtfStringBuilder: someone should write a separate Writeable impl for that. I also didn't do ZonedDateTime due to #330; IxdtfStringBuilder needs to borrow from the timezone. It could also be written to use a type that is effectively |
c814ae6
to
dfb9d67
Compare
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.
LGTM
I didn't do all of the APIs, just some of them.
I wasn't sure if I should make IxdtfStringBuilder writeable, or give it a
.writeable()
method that returns animpl Writeable
(the inner type). Open to opinions.