time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.11k stars 278 forks source link

Implement `Weekday::nth_next` #544

Closed Kinrany closed 1 year ago

Kinrany commented 1 year ago

Adds Weekday::nth, a fn(Weekday, u8) -> Weekday method.

This is similar to Iterator::nth: it returns the n-th next week day after the current one.

This is in place of four different methods with long names that would be opposites of number_from_monday, number_from_sunday, num_days_from_monday, num_days_from_sunday. I was bikeshedding the names in my mind and realized that making a single general method is likely more ergonomic, if only because of the amount of effort it was taking to write - and therefore understand - the fairly long descriptions of each method.

Closes https://github.com/time-rs/time/issues/542

codecov[bot] commented 1 year ago

Codecov Report

Merging #544 (26feb05) into main (c45264c) will increase coverage by 0.5%. The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main    #544     +/-   ##
=======================================
+ Coverage   95.3%   95.7%   +0.5%     
=======================================
  Files         78      79      +1     
  Lines       8563    8780    +217     
=======================================
+ Hits        8157    8406    +249     
+ Misses       406     374     -32     
Impacted Files Coverage Δ
time/src/weekday.rs 100.0% <100.0%> (ø)

... and 36 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Kinrany commented 1 year ago

I don't think I know what to do with those seemingly unrelated Clippy errors 😓

jhpratt commented 1 year ago

Don't worry about clippy. That's a side effect of there being a Rust release yesterday that includes new lints. However, code coverage is also failing, so presumably some more tests are needed.

Kinrany commented 1 year ago

Hmm, it's asking to cover an unreachable case. Wish there was a way to construct the enum from a raw u8 without bringing a whole extra package.

jhpratt commented 1 year ago

Alright. I'll have a concrete suggestion when I get to reviewing the code.

jhpratt commented 1 year ago

@Kinrany Would you mind addressing the requested changes above? Otherwise I'll likely end up implementing the request myself.

Kinrany commented 1 year ago

Sorry @jhpratt. Other things keep taking priority. I should be able to find a moment to do it eventually, but I wouldn't mind at all if you did reimplement it. I don't care about authorship, this is just something I wanted to have in the library.

Thank you for the hard work of maintaining the package, you rule 😄

jhpratt commented 1 year ago

Up to you! I don't have a release planned for anything else at the moment, so it's not like there's a big rush.

Kinrany commented 1 year ago

Thank you!