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

added astimedelta() to Duration #49

Closed
wants to merge 1 commit into from
Closed

added astimedelta() to Duration #49

wants to merge 1 commit into from

Conversation

mristin
Copy link

@mristin mristin commented Mar 5, 2018

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 94.466% when pulling 9dd26b3 on mristin:added-astimedelta-to-Duration into ce635a7 on gweis:master.

@mristin
Copy link
Author

mristin commented Mar 11, 2018

Hi!
Any chance you might consider merging this change soon (in the next 2-3 weeks)? I would be very thankful for a response, so that we can either wait for this feature in your package or implement it in our code base independently.

@gweis
Copy link
Owner

gweis commented Mar 11, 2018

Hi,

To correctly transform a Duration to a timedelta you need a reference date. Otherwise you are just guessing, that a year has 365 days, a month has 30 days, which is not always the case.

There was a discussion around this in #42. (There are a couple of other suggestions in that thread that should probably make it into the library). However, I believe the cleanest way would be to use a reference date to convert to a timedelta in a generic way. Otherwise if you make assumptions about the length of a year or a month, you'd be probably better off to be explicit in your code, rather than hiding it in a library.

@mristin
Copy link
Author

mristin commented Apr 6, 2018

Hi @gweis
In most cases where we use durations they are stored in the config files and indicate the life time of certain objects. I assume this is not such a rare use case and could find general application. So for us the average month and year are fine.

What about having totimedelta() with both start and end None return the time delta with the average month and year?

@mristin
Copy link
Author

mristin commented Jun 2, 2018

Hi @gweis ,
Have you had a chance to consider the arguments to totimedelta()? Or what about adding the function to_timedelta_avg()? Please let me know what you prefer, and I'll gladly make a new pull request.

@gweis
Copy link
Owner

gweis commented Jul 5, 2018

Hi sorry for the late reply,

I have never seen a use case where I had a precise duration and only needed an estimate to what date that would lead me.

Wouldn't it be the same if you supply a start date of datetime.now() or whatever random start date you want? My expectation would be that if something has a life time of a month, it would be from 1st to 1st. Otherwise the life time should be represented as days in the first place.

@mristin
Copy link
Author

mristin commented Aug 1, 2018

Hi @gweis ,
Please also apologize for the late reply from my side -- I missed your comment in my email log and assumed wrongly there was no activity on this pull request.

I have never seen a use case where I had a precise duration and only needed an estimate to what date that would lead me.

Think of the use case where the duration models a sort of age (e.g., how long to keep some cached data, or how long to keep the private data in the database etc.). You would always use the same duration even though the reference point would constantly change. While I agree that it is perfectly possible to store the duration in days, writing P180D is way less practical than P3M. If it's five years then the period becomes quite unreadable: P1825D. The month in this context obviously does not refer to a calendar month.

We can also close the issue if you feel strongly about this use case and think that it would add clutter to the library.

@gweis
Copy link
Owner

gweis commented Aug 2, 2018

You would always use the same duration even though the reference point would constantly change

That's exactly my point. What data retention do you promise here? P3M can be seen as something from 90 to 92 days. If you say a month is 30 days, then you actually promise 90 days.

Same with years. Do you promise 365 days or 366 days when you say P1Y ?

totimedelta(datetime.now()), will always give you a timedelta that's correct with the given reference date.

Or think of birthdays in terms of age. Someone has lived P10Y years, and you add one year. By just estimating how long this period is, you'll probably send out birthday greeting cards on the wrong day. (not by much but still wrong).

Fell free to re-open in case if you think I am getting something really, wrong here.

@gweis gweis closed this Aug 2, 2018
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

4 participants