Skip to content
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

Add new datetime field set builder #5944

Merged
merged 10 commits into from
Dec 21, 2024
Merged

Add new datetime field set builder #5944

merged 10 commits into from
Dec 21, 2024

Conversation

sffc
Copy link
Member

@sffc sffc commented Dec 20, 2024

Part of #5940

pub fn build_time(mut self) -> Result<TimeFieldSet, BuilderError> {
let time_field_set =
TimeFieldSet::T(fieldsets::T::take_from_builder(&mut self, DEFAULT_LENGTH));
self.check_options_consumed()?;
Copy link
Member

Choose a reason for hiding this comment

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

praise: clever

@@ -236,6 +238,18 @@ macro_rules! impl_marker_with_options {
$(time_precision: yes_to!(options.time_precision, $timeprecision_yes),)?
}
}
#[allow(unused)]
pub(crate) fn take_from_builder(
Copy link
Member

Choose a reason for hiding this comment

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

praise: also clever

Copy link
Member

Choose a reason for hiding this comment

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

docs nit: since this is macro generated, docs explaining its behavior would be nice

pub year_style: Option<YearStyle>,
}

const DEFAULT_LENGTH: Length = Length::Medium;
Copy link
Member

Choose a reason for hiding this comment

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

docs nit: should explain why this is here instead of in the macro


impl FieldSetBuilder {
pub fn build_date(mut self) -> Result<DateFieldSet, BuilderError> {
let date_field_set = match self.date_fields.take() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: should explain why the time precision is being handled the way it is


#[derive(Debug, Copy, Clone, PartialEq, Eq, Default)]
#[non_exhaustive]
pub struct FieldSetBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

docs nit: should have docs explaining use, and talking qualitatively about which combinations are valid. users should ideally not have to use trial and error to figure out what fields they need to set for their use case

@sffc sffc mentioned this pull request Dec 21, 2024
@sffc sffc changed the title Initial draft of fieldset builder Add new datetime field set builder Dec 21, 2024
@sffc sffc requested a review from Manishearth December 21, 2024 05:36
@sffc sffc marked this pull request as ready for review December 21, 2024 05:36
@sffc sffc requested a review from zbraniecki as a code owner December 21, 2024 05:36
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

praise: good docs

@sffc sffc merged commit 9a60869 into unicode-org:main Dec 21, 2024
28 checks passed
@sffc sffc deleted the fset-builder branch December 21, 2024 23:31
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