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

Preserve timezone when converting DateTime to OFX. #115

Merged
merged 2 commits into from
Feb 2, 2021

Conversation

stoklund
Copy link
Contributor

@stoklund stoklund commented Feb 2, 2021

Some OFX servers are particular about the formatting of the timezone in
<DTSTART> elements, and they reject the [0:GMT] timezone produced by
ofxtools.

Preserve the timezone of the datetime objects used when generating the OFX
timestamps to avoid this problem.

See discussion in #114.

This is a mechanical search and replace in preparation for the next commit which
changes the formatting used for the UTC timezone.
Some OFX servers are particular about the formatting of the timezone in
`<DTSTART>` elements, and they reject the `[0:GMT]` timezone produced by
ofxtools.

Preserve the timezone of the `datetime` objects used when generating the OFX
timestamps to avoid this problem. Always format the UTX offset with a leading
`+` or `-`, even though the OFX spec permits the `+` to be dropped.

Move the timestamp formatting into a `format_datetime()` which is shared by the
`DateTime` and `Time` classes. This also ensures that millisecond rounding is
handled correctly for both.
@stoklund stoklund changed the title [WIP] Preserve timezone when converting DateTime to OFX. Preserve timezone when converting DateTime to OFX. Feb 2, 2021
@stoklund
Copy link
Contributor Author

stoklund commented Feb 2, 2021

Updated the PR with some tests, and also shared the formatting code with the Time class to make things consistent.

Some timezones in Australia and India have a fractional hour offset from UTC. The OFX 1.6 standard says:

Note that times zones are specified by an offset and optionally, a time zone name. The offset defines the time zone. Valid offset values are in the range from –12 to +12 for whole number offsets. Formatting is +12.00 to -12.00 for fractional offsets, plus sign may be omitted.

I interpret this as a decimal fraction, so India's +0530 offset becomes [+5.50:IST]. It looks like the existing TIME_REGEX interprets this as a +h.mm format containing minutes rather than a decimal fraction. It's not clear to me which interpretation is correct.

I don't know if OFX is used in India or Australia.

@csingley
Copy link
Owner

csingley commented Feb 2, 2021

I interpret this as a decimal fraction, so India's +0530 offset becomes [+5.50:IST].

I don't think the OFX spec writers were precise enough with their language that their choice of the word "fraction" should outweigh the huge driving force to fall in line with ISO 8601 conventions.

The OFXv1 DTDs (SGML doc definitions) are sloppy, but the date & time regexes in ofxtools.Types were copied directly from the OFXv2 XML schema, which makes it explicit that the spec intends ISO 8601 semantics.

Don't worry about it though, usage of OFX outside of America is low. I'll fix this part.

Thanks very much for fixing up the tests, it's appreciated.

@csingley csingley merged commit a170a4b into csingley:master Feb 2, 2021
@stoklund
Copy link
Contributor Author

stoklund commented Feb 2, 2021

I don't think the OFX spec writers were precise enough with their language that their choice of the word "fraction" should outweigh the huge driving force to fall in line with ISO 8601 conventions.

Yeah, I agree. Decimal fractions would be silly.

I pushed an updated version at https://github.com/stoklund/ofxtools/tree/timezone which uses minutes.

@csingley
Copy link
Owner

csingley commented Feb 2, 2021

I was just sitting down to overhaul that old logic to use datetime division... thanks for doing it. A lot of code in this module dates back to the days of Python 2.

Now that we're requiring recent versions of Python, I guess we can now employ f-strings too!

@csingley
Copy link
Owner

csingley commented Feb 2, 2021

Done in 8bee7fe

Thanks again.

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