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

OffsetDateTime::replace_time? #256

Closed
djahandarie opened this issue May 7, 2020 · 3 comments · Fixed by #284
Closed

OffsetDateTime::replace_time? #256

djahandarie opened this issue May 7, 2020 · 3 comments · Fixed by #284
Assignees
Labels
A-core Area: anything not otherwise covered C-feature-request Category: a new feature (not already implemented) E-easy Not much experience needed.
Milestone

Comments

@djahandarie
Copy link

I find myself writing code like this:

my_offset_date_time
  .date()
  .with_time(time!(05:00))
  .assume_offset(my_offset_date_time.offset())

which, while is fairly explicit about what is going on, feels a little long compared to

my_offset_date_time.replace_time(time!(05:00))

(I figured with_time is not appropriate thing to call this, because the rest of the library uses that for combining things rather than replacing parts of things. I imagine set_ could also be a decent name, though that very slightly suggests more of a &mut self interface to me, as opposed to just a self)

And in fact, since it seems that "replacing parts of things" is generally not a pattern exposed anywhere in the library, this would likely open up the pandora's box of "but I want to replace the hour of this Time" etc. etc., so I could understand why one might not want to go down this route.

That said, I'd find this quite ergonomic, so here's my shot at suggesting it!

@jhpratt
Copy link
Member

jhpratt commented May 7, 2020

Something along the lines of set_ might be feasible. I'll have to check on the interop between Copy types and &mut self methods, as I'm not sure off the to of my head.

I'm definitely open to this in some form, though, as it's a pattern I have thought about previously.

@jhpratt jhpratt added the C-feature-request Category: a new feature (not already implemented) label May 7, 2020
@jhpratt jhpratt added A-core Area: anything not otherwise covered E-easy Not much experience needed. labels May 16, 2020
@jhpratt
Copy link
Member

jhpratt commented Oct 1, 2020

For those coming here from TWIR, I thought this issue would be a good one to tackle for Hacktoberfest!

What I'd like to see in a PR is a number of methods (on Time, Date, PrimitiveDateTime, and OffsetDateTime) with the signature fn replace_foo(self) -> Self, not a mutating version as mentioned in my previous comment. All components should be able to be replaced, even if they are not directly present in the struct definition. OffsetDateTime replacements should operate similarly to the getters, in that the offset is taken into account before passing it through to the PrimitiveDateTime.

As with any change, documentation and tests should be present. Don't worry about what is done when, as I can clean up the commit history before merging.

Note that any PR doing this should be based on the v0.3 branch, not main. v0.3 is relatively stable with the exception of formatting.

@Ask-0
Copy link
Contributor

Ask-0 commented Oct 3, 2020

I would like to claim this one.

@jhpratt jhpratt linked a pull request Oct 7, 2020 that will close this issue
@jhpratt jhpratt closed this as completed Nov 4, 2020
@jhpratt jhpratt added this to the v0.3 milestone Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: anything not otherwise covered C-feature-request Category: a new feature (not already implemented) E-easy Not much experience needed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants