Skip to content

Add Writeable getters for some types, use in FFI#340

Merged
nekevss merged 1 commit into
boa-dev:mainfrom
Manishearth:writeable
Jun 6, 2025
Merged

Add Writeable getters for some types, use in FFI#340
nekevss merged 1 commit into
boa-dev:mainfrom
Manishearth:writeable

Conversation

@Manishearth

Copy link
Copy Markdown
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.

@nekevss nekevss left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/parsers.rs
}
}

impl<'a> Writeable for IxdtfStringBuilder<'a> {

@nekevss nekevss Jun 5, 2025

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

Comment thread temporal_capi/src/plain_date_time.rs Outdated
@Manishearth Manishearth force-pushed the writeable branch 2 times, most recently from 77382b4 to 258bbdf Compare June 5, 2025 21:08
@Manishearth

Copy link
Copy Markdown
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

@nekevss nekevss left a comment

Copy link
Copy Markdown
Member

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
@Manishearth Manishearth deleted the writeable branch August 2, 2025 06:06
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