Skip to content

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

Merged
merged 1 commit into from
Jun 6, 2025

Conversation

Manishearth
Copy link
Contributor

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 an impl Writeable (the inner type). Open to opinions.

Copy link
Member

@nekevss nekevss left a 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> {
Copy link
Member

@nekevss nekevss Jun 5, 2025

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.

Copy link
Contributor Author

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.

@Manishearth Manishearth force-pushed the writeable branch 2 times, most recently from 77382b4 to 258bbdf Compare June 5, 2025 21:08
@Manishearth
Copy link
Contributor Author

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 (time_zone, IxdtfStringBuilder).

@Manishearth Manishearth force-pushed the writeable branch 2 times, most recently from c814ae6 to dfb9d67 Compare June 5, 2025 21:15
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nekevss nekevss merged commit ee39f9f into boa-dev:main Jun 6, 2025
8 checks passed
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.

2 participants