OFX Spec Compliance Fixes #332
Conversation
src/ofxstatement/ofx.py
Outdated
| encoded = str(encoded, "utf-8") | ||
| encoding = "cp1252" | ||
| encoded = etree.tostring(et.getroot(), encoding) | ||
| encoded = str(encoded, encoding) |
There was a problem hiding this comment.
I assume this breaks any outputs with characters outside of cp1252 encoding (IOW, this will probably break ofxstatement for a lot of users, including me). This doesn't seem right.
Regarding the encoding, you seem to refer to some ancient OFX spec from 1999 in #327 (comment). We shouldn't be stuck with such an old version in 2025.
There was a problem hiding this comment.
I suppose it depends on the support by other tools. If OFX 1.0.2 is more widely supported, it makes sense to stick with that unless there is a reason to upgrade.
The 1.0.2 spec talks about ENCODING:UNICODE, and the OFX 2.2 spec implies that this referred to UTF-8. Most tools (e.g. GnuCash) support UNICODE, we can just use that. OfxSharp which is used by Denaro does not support it, but I think this should be fixed upstream, not worked around here.
FYI, we are not the only ones struggling with OFX encoding: https://github.com/wesabe/fixofx/blob/1792d94697af682ca1d4a75cfefe98465d95a288/lib/ofx/document.py#L27-L42 😅
There was a problem hiding this comment.
Yeah, I agree, just having it as UNICODE would be fine. From what I remember I didn't specifically chose OFX 1.0.2 for compatibility, but it was just what was in some sample file I used in initial implementation. So we can be flexible here, as long as consuming tools are ok with that.
For this case, I'm fine if the encoding would be UNICODE instead of UTF-8.
There was a problem hiding this comment.
Ok, I noticed that the encoding is already configurable, so I made the OFX writer respect that. If "utf-8" is used, it will use ENCODING:UNICODE and CHARSET:NONE, if cp1252 is used it will use ENCODING:USASCII and CHARSET:1252.
For other charsets, it will try to handle this gracefully by using a nonstandard charset. Alternatively, we could raise an error.
Holzhaus
force-pushed
the
ofx-spec-compliance
branch
from
afba71f to
e21fde6
Compare
The variable `encoded` actually does not contain `encoded` data, it's a Python unicode string. This was confusing.
Holzhaus
force-pushed
the
ofx-spec-compliance
branch
2 times, most recently
from
edcb14c to
ebfe884
Compare
Holzhaus
force-pushed
the
ofx-spec-compliance
branch
from
ebfe884 to
febd0c3
Compare
|
I checked on my files and it seems good. So, just run |
|
Will do tomorrow (if you don't want to wait you should also be able to do it yourself and push to this branch). By the way, have you considered using https://pre-commit.com/ instead of |
|
pre-commit is a good idea. I'll add it. |
|
Okay, tests look good. I'm merging it. Thanks a lot! Do you want a new release? |
Not urgent to me, I'm using ofxstatement in a venv anyway. 🙂 |
This makes it possible to import the output of ofxstatement with Denaro. See #327 (comment) for detailed explanations.
Fixes #327.