Skip to content

Conversation

@jralls
Copy link
Collaborator

@jralls jralls commented Mar 26, 2021

See GnuCash Bug 797848 and Bug

This PR has two commits. The first one implements the changes discussed with @benoitg in the first bug; the second is a general cleanup of ofxdate_to_time_t that's easier to read and might even be a bit faster. If you don't like the second by all means just cherry-pick the first.

jralls added 2 commits March 25, 2021 17:32
Sets the default time to 1059 UTC instead of 1159 local because
1059 UTC is date-stable for all timezones except -12 and +14 whereas
1159 local is date stable for 12 hours relative to the current time
zone, which is less stable except for timezone close to the prime
meridian.

Addresses underlying causes of GnuCash bugs
https://bugs.gnucash.org/show_bug.cgi?id=636340 and
https://bugs.gnucash.org/show_bug.cgi?id=797848
Take a const string& to avoid the copy.
Reorder the conditions to reduce nesting and improve flow.
Remove unused variables.
memset the struct tm to 0 instead of to the current local time.
Use std::string::empty() instead of std::string::size() == 0.
In the empty string case just return time(NULL) instead of converting
that to a struct tm and then back to a time_t.
Move declarations of variables to point of use and initialize instead of
assign.
Apply the timezone offset directly to struct tm an call timegm instead
of messing around with the user's timezone and worrying about  whether
it's in DST. checked_mktime is no longer needed.
Rewrite the doxygen comment to be more concise.
@cstim
Copy link
Member

cstim commented Apr 23, 2021

(Sorry for the long delay) Looks very good. Especially thanks for the refactoring! If that other time-of-day works much better for users, we indeed should use it. Thanks!

@cstim cstim merged commit f42a6a7 into libofx:master Apr 23, 2021
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