Skip to content

Updated years_from_now to not blow up on leap days - #95

Merged
partim merged 2 commits into
NLnetLabs:masterfrom
dadepo:master
Mar 3, 2020
Merged

Updated years_from_now to not blow up on leap days#95
partim merged 2 commits into
NLnetLabs:masterfrom
dadepo:master

Conversation

@dadepo

@dadepo dadepo commented Feb 29, 2020

Copy link
Copy Markdown
Contributor

Tried to start Krill just now and got a panic message:

thread 'main' panicked at 'No such local time', ~/.cargo/registry/src/github.com-1ecc6299db9ec823/chrono-0.4.10/src/offset/mod.rs:173:34
stack backtrace:

Found the cause to be due to the implementation of Time::years_from_now which was simply adding the numeric value of years to the current year. Things go wrong if this is done on a leap day, for example adding 1 year today would give 2021-02-29 which is invalid since 2021 is not a leap year.

This PR seeks to fix that. It calculates the next years by adding 365 days. This means a year from a leap day would now result to February 28.

@dadepo

dadepo commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

Just noticed Travis isn't happy with these changes. I looked at the error logs and not sure if its grumpiness is due to these changes or something else.

If you cool with this PR, do let me know what I need to do to make Travis happy again, so I can fix before merging

@partim

partim commented Mar 2, 2020

Copy link
Copy Markdown
Member

Thank you for the pull request and sorry for that bug!

Looks like the CI failure is just a Travis thing. I restarted it, let’s see if it is happier now.

That said, I m wondering if adding years*356 days is the best way here. If you add ten years, the result will (in most cases) be two days off. While that probably won’t be an issue, it might be confusing. Maybe it will be better to stick to the current method but just add a February 29 exceptions (by simply always changing it to February 28 without further checks)?

@dadepo

dadepo commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

Maybe it will be better to stick to the current method but just add a February 29 exceptions (by simply always changing it to February 28 without further checks)?

Sure, I would incorporate your feedback. Hopefully by this evening.

@dadepo

dadepo commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

Also was not sure if February 28 should be the date, a year from a leap day, or March 1st. Not sure what the general practice is. Any thoughts?

@partim

partim commented Mar 2, 2020

Copy link
Copy Markdown
Member

I suspect you can argue forever about that. We should probably just pick one, document it, and move on. If someone needs fine-grained control, they can always write their own version.

@dadepo

dadepo commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

Cool. I'll stick with Feb. 28

@partim

partim commented Mar 2, 2020

Copy link
Copy Markdown
Member

Not sure what’s up with Travis. I think you can ignore it for now.

@dadepo

dadepo commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

Not sure what’s up with Travis. I think you can ignore it for now.

Ok, no problem.

Maybe it will be better to stick to the current method but just add a February 29 exceptions (by simply always changing it to February 28 without further checks)?

By the way, we can't always reset to Feb. 28 when we get Feb. 29, since, if the resulting year is indeed a leap year, then we need to keep it at Feb. 29.

'Would keep all these in mind when I revisit the PR

@partim

partim commented Mar 2, 2020

Copy link
Copy Markdown
Member

Strictly speaking you are right, but I think to keep the code simple, it is probably fine to just flip to the 28 always and be done. This is enough of a corner case to just document the behaviour.

@dadepo

dadepo commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

... This is enough of a corner case to just document the behaviour.

Done.

@partim

partim commented Mar 3, 2020

Copy link
Copy Markdown
Member

Looks good now. Thank you for debugging and fixing!

@partim
partim merged commit 39e1b88 into NLnetLabs:master Mar 3, 2020
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.

2 participants