-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
src/builtins/core/date.rs
Outdated
/// 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 | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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
...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
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:
After:
If this doesn't make much sense and it's preferred to keep the current method of
StructExpressions