Skip to content

implement utility methods on partial structs #206

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 5 commits into from
Feb 24, 2025
Merged

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Feb 23, 2025

This PR adds methods with the goal of making Partials structs a bit more ergonomic to work with rather than using only struct expressions.

Before:

let day = Some(24);
let date = PartialDate {
    day,
    ..Default::default()
};

After:

let day = Some(24);
let date = PartialDate::default().with_day(day);

If this doesn't make much sense and it's preferred to keep the current method of StructExpressions

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

I don't mind the addition.

@jedel1043 jedel1043 added C-enhancement New feature or request C-api Changes related to the public API labels Feb 24, 2025
Copy link
Member

@jedel1043 jedel1043 left a 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 sense. Moving structs is much safer for FFI than passing references, since it's harder to preserve the mutable borrow XOR shared borrow rule.

Comment on lines 128 to 165
/// Convenience methods for building a `PartialDate`
impl PartialDate {
pub const fn with_era(mut self, era: Option<TinyAsciiStr<19>>) -> Self {
self.era = era;
self
}

pub const fn with_era_year(mut self, era_year: Option<i32>) -> Self {
self.era_year = era_year;
self
}

pub const fn with_year(mut self, year: Option<i32>) -> Self {
self.year = year;
self
}

pub const fn with_month(mut self, month: Option<u8>) -> Self {
self.month = month;
self
}

pub const fn with_month_code(mut self, month_code: Option<TinyAsciiStr<4>>) -> Self {
self.month_code = month_code;
self
}

pub const fn with_day(mut self, day: Option<u8>) -> Self {
self.day = day;
self
}

pub const fn with_calendar(mut self, calendar: Calendar) -> Self {
self.calendar = calendar;
self
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about this, can you add a const new constructor? Having all of these be const is useful, but they're less useful because we don't have a way to ergonomically create a const PartialDate without having to manually set all fields to None...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to add a const new as well. But it requires a const calendar constructor, which was ultimately the blocker. I'll play around with it a bit more.

@nekevss nekevss requested a review from jedel1043 February 24, 2025 02:53
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looks good!

@nekevss nekevss merged commit 4cfa2da into main Feb 24, 2025
9 checks passed
@nekevss nekevss deleted the impl-partial-methods branch February 24, 2025 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-api Changes related to the public API C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants