Skip to content

implement utility methods on partial structs#206

Merged
nekevss merged 5 commits intomainfrom
impl-partial-methods
Feb 24, 2025
Merged

implement utility methods on partial structs#206
nekevss merged 5 commits intomainfrom
impl-partial-methods

Conversation

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

Comment thread src/builtins/core/duration.rs
@jedel1043 jedel1043 added C-enhancement New feature or request C-api Changes related to the public API labels Feb 24, 2025
Copy link
Copy Markdown
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 thread src/builtins/core/date.rs
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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