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

Fixed year handling in macOS date formats #477

Merged
merged 4 commits into from Feb 4, 2017

Conversation

Projects
None yet
2 participants
@lenlo
Contributor

lenlo commented Jan 29, 2017

I have set the system date format to yyyy-MM-dd on my Mac, but when I started using moneyGuru, I found that it would produce dates of the form dd-MM-yyyy instead. The reason for this is that macOS (and possibly others) use a single 'y' to denote a non-zerofilled complete year. moneyGuru didn't recognize this and defaulted to its fallback format instead.

Fixed by substituting 'yyyy' for 'y' before parsing the format string. This will give us zero filled centuries for years < 1000, but that's about as close as we can get giving strftime's limitations. Hopefully, there's no real need to process historic accounting records for Roman times anyway.

Fixed year handling in macOS date formats
macOS (and possibly others) use 'y' to denote a complete year in date
formats. moneyGuru didn't recognize this and defaulted the standard US date
format MM/dd/yy instead. Fixed by substituting 'yyyy' for 'y'. This will
give us zero filled centuries for years < 1000, but that's about as close as
we can get giving strftime's limitations. Hopefully, there's no real need to
process historic accounting records for Roman times anyway.

@hsoft hsoft self-requested a review Jan 29, 2017

@hsoft

Thanks! I can confirm the problem and the fix. moneyGuru has very good test coverage and there's usually always an automated test coming with features like this. Would you mind writing one? I'd be happy to assist if you're uncomfortable with this.

Show outdated Hide outdated core/model/date.py
@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Jan 31, 2017

Contributor

Feel free to add a test case. I'm not very familiar with how the unit tests work.

Contributor

lenlo commented Jan 31, 2017

Feel free to add a test case. I'm not very familiar with how the unit tests work.

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Jan 31, 2017

Owner

I don't know when I'll have time for this myself, but in any case, I'll be glad to merge this as soon as it has unit tests.

Owner

hsoft commented Jan 31, 2017

I don't know when I'll have time for this myself, but in any case, I'll be glad to merge this as soon as it has unit tests.

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Feb 3, 2017

Contributor

Added a test case that I hope is appropriate.

Contributor

lenlo commented Feb 3, 2017

Added a test case that I hope is appropriate.

@hsoft

hsoft approved these changes Feb 4, 2017

Awesome, thanks!

@hsoft hsoft merged commit dc52847 into hsoft:master Feb 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment