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

Add elapsed_years to Date and DateTime #557

Merged
merged 34 commits into from Jun 8, 2022
Merged

Conversation

yozhgoor
Copy link
Contributor

@yozhgoor yozhgoor commented Apr 27, 2021

Thanks for contributing to chrono!

  • Have you added yourself and the change to the changelog? (Don't worry
    about adding the PR number)
  • If this pull request fixes a bug, does it add a test that verifies that
    we can't reintroduce it?

Related to #416

I have make this PR for retrieve easily an age from a birthdate
The API is similar to std::time::Instant

@quodlibetor
Copy link
Contributor

It seems like this would work better as a method on chrono::Duration?

@yozhgoor
Copy link
Contributor Author

I need to use a date here, because years are not constant. A year is on average equals to 52.1428571 weeks so this will be really less accurate on chrono::Duration.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
yozhgoor and others added 3 commits June 13, 2021 19:20
Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
@yozhgoor
Copy link
Contributor Author

Thanks @cecton for the spell checking :D

@yozhgoor
Copy link
Contributor Author

yozhgoor commented Jun 13, 2021

I don't know what i do with this PR btw 🤔
It's not a problem if this don't fit with the API, at that point we can just close.
I just wanted to solve a problem I had and a few others as well (see #416 (comment) for example).
I don't have the level to try to calculate the elapsed years on Duration and this would also bring big changes to the API
This PR is the most accurate i can do for the moment 😄

@Milo123459
Copy link
Member

I think if you make it a method like Brandon said, this PR could get some work done on it and possibly get merged.

@cecton
Copy link
Contributor

cecton commented Oct 26, 2021

I think if you make it a method like Brandon said, this PR could get some work done on it and possibly get merged.

It can't be a method of duration because the exact date must be known.

@cecton
Copy link
Contributor

cecton commented Oct 26, 2021

I need to use a date here, because years are not constant. A year is on average equals to 52.1428571 weeks so this will be really less accurate on chrono::Duration.

☝️

@Milo123459
Copy link
Member

Woops didn't see

@cecton
Copy link
Contributor

cecton commented Oct 27, 2021

Thanks for the merge! ...though it doesn't seem the maintainer is interested in this API feature.

@cecton
Copy link
Contributor

cecton commented Oct 27, 2021

Ah cool then 🤗 That PR was staling for a while now. I understood it as a "no".

I'm afraid you failed the conflict resolution. I added a suggestion to fix it (it's not my PR, I don't think I can push on it)

CHANGELOG.md Outdated Show resolved Hide resolved
Milo123459 and others added 2 commits October 27, 2021 22:06
Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
@Milo123459
Copy link
Member

This PR sadly can't be merged until the tests are fixed, and it looks like some outdated methods are being used. Not sure though.

@yozhgoor
Copy link
Contributor Author

Thanks @cecton and @Milo123459
I updated my fork for this PR and made some change. I have try to fix tests.

@cecton
Copy link
Contributor

cecton commented Nov 12, 2021

@Milo123459 this seems ready now 🚀

@yozhgoor
Copy link
Contributor Author

I fix a conflict in CHANGELOG.md and i think i resolved the issues with Utc::now and Utc::today.
(I discovered the script to launch the CI locally too)

@yozhgoor
Copy link
Contributor Author

yozhgoor commented Dec 8, 2021

@Milo123459 Can we give it another shot before new changes ?
Thanks

@yozhgoor
Copy link
Contributor Author

So, this seems to be blocked again and I'm not sure why. What needs to be done to move forward?

This feature cannot be made on Duration because the exact date needs to be known to calculate the number of years exactly. Otherwise it would be an estimation of the numbers of years. My code here gives the exact number of years.

Do we want this feature or we don't want this feature? I would prefer to get a clear answer if possible.
Otherwise it feels like I'm being ignored without any reason provided.

CHANGELOG.md Outdated
@@ -744,4 +745,3 @@ and replaced by 0.2.25 very shortly. Duh.)
## 0.1.0 (2014-11-20)

The initial version that was available to `crates.io`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this useless change

src/date.rs Outdated
@@ -275,6 +276,20 @@ impl<Tz: TimeZone> Date<Tz> {
pub fn naive_local(&self) -> NaiveDate {
self.date
}

/// Retrieve the elapsed years from now to the given [`Date`].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consistency with other's function docs

Suggested change
/// Retrieve the elapsed years from now to the given [`Date`].
/// Retrieves the elapsed years from now to the given [`Date`].

@djc
Copy link
Contributor

djc commented Jun 8, 2022

Unfortunately std::convert::TryFrom is not available in our MSRV target, so this needs some work.

@yozhgoor yozhgoor marked this pull request as draft June 8, 2022 08:49
@yozhgoor yozhgoor marked this pull request as ready for review June 8, 2022 09:22
@djc
Copy link
Contributor

djc commented Jun 8, 2022

Sorry, can you rebase this again to pull in the fixes from #706?

@djc djc merged commit 1d33cbc into chronotope:main Jun 8, 2022
@yozhgoor yozhgoor deleted the elapsed-years branch June 8, 2022 11:09
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.

None yet

5 participants