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

Implement Weekday::nth_next #544

Merged
merged 2 commits into from
Apr 20, 2023
Merged

Implement Weekday::nth_next #544

merged 2 commits into from
Apr 20, 2023

Conversation

Kinrany
Copy link
Contributor

@Kinrany Kinrany commented Jan 26, 2023

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 #542

@codecov
Copy link

codecov bot commented Jan 26, 2023

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

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

@Kinrany
Copy link
Contributor Author

Kinrany commented Jan 27, 2023

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

@jhpratt
Copy link
Member

jhpratt commented Jan 27, 2023

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
Copy link
Contributor Author

Kinrany commented Jan 29, 2023

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
Copy link
Member

jhpratt commented Jan 29, 2023

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

@jhpratt jhpratt added the C-keep-open Category: should not be closed due to inactivity label Jan 29, 2023
benchmarks/weekday.rs Outdated Show resolved Hide resolved
tests/weekday.rs Outdated Show resolved Hide resolved
time/src/weekday.rs Outdated Show resolved Hide resolved
time/src/weekday.rs Outdated Show resolved Hide resolved
time/src/weekday.rs Outdated Show resolved Hide resolved
@jhpratt jhpratt removed the C-keep-open Category: should not be closed due to inactivity label Feb 16, 2023
@jhpratt
Copy link
Member

jhpratt commented Mar 19, 2023

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

@Kinrany
Copy link
Contributor Author

Kinrany commented Mar 19, 2023

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
Copy link
Member

jhpratt commented Mar 19, 2023

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.

@jhpratt jhpratt force-pushed the main branch 2 times, most recently from b9c3eee to 4507d07 Compare April 18, 2023 09:51
@jhpratt jhpratt changed the title Implement Weekday::nth Implement Weekday::nth_next Apr 20, 2023
@jhpratt jhpratt enabled auto-merge (squash) April 20, 2023 10:29
@jhpratt jhpratt merged commit 6795714 into time-rs:main Apr 20, 2023
21 checks passed
@Kinrany
Copy link
Contributor Author

Kinrany commented Apr 20, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weekday from number
2 participants