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

New Ranges Feature #524

Closed ferdinandkeller closed 1 year ago

ferdinandkeller commented 1 year ago

I've started to implement proper ranges for the library. This pull request is not one that is ready to be merged with the main branch. Many functions don't have an implementation yet, and it still lacks some features I would like to add. But I would like some feedback before progressing in the direction I am going.

What is new

Here is a quick summary of what is present:

Important notes about ranges:

Questions I might have

Should ranges implement the Copy trait ? The struct Range<OffsetDateTime> never changes, and doesn't implement the Iterator trait, so there shouldn't be any problem with this. I would like your feedback on the idea.

EDIT: You can't implement the Copy trait, as the Range struct is not defined inside the crate.

Coming improvements

codecov[bot] commented 1 year ago

Codecov Report

Merging #524 (3e9c728) into main (01ca01a) will decrease coverage by 1.6%. The diff coverage is 80.8%.

@@           Coverage Diff           @@
##            main    #524     +/-   ##
=======================================
- Coverage   98.4%   96.8%   -1.6%     
=======================================
  Files         76      77      +1     
  Lines       8215    9091    +876     
=======================================
+ Hits        8082    8797    +715     
- Misses       133     294    +161     
Impacted Files Coverage Δ
time/src/lib.rs 100.0% <ø> (ø)
time/src/offset_date_time_range.rs 76.0% <76.0%> (ø)
time/src/offset_date_time.rs 99.7% <99.4%> (-0.3%) :arrow_down:
time/src/util.rs 100.0% <0.0%> (ø)
time/src/duration.rs 100.0% <0.0%> (ø)
time/src/format_description/parse/lexer.rs 100.0% <0.0%> (ø)
time/src/format_description/parse/ast.rs 94.0% <0.0%> (+0.2%) :arrow_up:
time/src/format_description/parse/mod.rs 97.0% <0.0%> (+0.4%) :arrow_up:
time/src/sys/local_offset_at/unix.rs 40.0% <0.0%> (+14.4%) :arrow_up:

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

jhpratt commented 1 year ago

Just a heads up, it'll probably be a few days before I get to looking at this.

ferdinandkeller commented 1 year ago

Don't look too deep in the methods implementation for now. There will be some work later on to make sure all the functions work properly, but for now many are not implemented, and some are implemented with errors. But even though I am doing a lot of fixes right now, the way the feature behaves hasn't changed much. I hope you find the feature coherent and pleasant to use.

jhpratt commented 1 year ago

Yeah, when I get around to it (possibly this coming weekend), I'll be looking at the public API, not the implementation.

jhpratt commented 1 year ago

After a quick review of the public API, I'm honestly impressed that you managed to get this working — it's a complicated thing to manage. With that said, I am (unfortunately) closing this pull request, as I feel it's far too large for me to manage at this point in time. I am quite busy working on language-related matters, so any issues that pop up would require me to spend time that I don't have.

Thank you for your contribution regardless! If/when I come around to having iterator helpers, I will definitely look at this code for guidance.