Warn (or raise in strict mode) when a number is too wide for its dbf field - #442
Warn (or raise in strict mode) when a number is too wide for its dbf field#442gaoflow wants to merge 1 commit into
Conversation
… number is too wide for its dbf field _record capped over-wide N and F values with format(...)[:size].rjust(size), which pads back to exactly size, so the len(encoded) != size guard could never fire. Truncating a decimal representation writes a different number, e.g. an N field of size 5 stored 123456789 as b"12345", and N(6, decimal=2) stored 12345.67 as b"12345.", with no warning even in strict mode. Both numeric sub-paths now share _pack_dbf_number, which warns (or raises the new DbfNumericDataLoss in strict mode) as the C and M paths do, without changing the bytes written. DbfStringDataLoss and DbfNumericDataLoss share a new DbfDataLoss base class.
Let me know if you require otherwise, but I strongly suggest simply using a wide enough field for the particular data source. Or use a modern, non-obsolete data format that stores floats as floats, instead of as ascii (like .dbfs do).
If fixing this is a biggie, or even just a nice-to-have for anyone, start a discussion and lets talk, human to human, and figure out what to do together, instead of just asking an LLM. |
|
Fair callout, and honest answer since you asked directly: yes, I use an AI assistant for this — the digging and the patch — under my direction, and I'm accountable for what gets posted. Should have opened a discussion first instead of a PR; sorry for the noise. On the substance: you're right that the tests/docstrings are too heavy for a one-line warning on a private hot-loop function, and if you'd rather not support truncation warnings at all that's a legitimate call — it's your API surface. Happy to trim it to the essentials if there's still interest, otherwise feel free to close. |
Writer._recordfits numbers into "N"/"F" fields withformat(...)[:size].rjust(size), padding back to exactlysizeso the trailinglen(encoded) != sizeguard can never fire. A truncated decimal representation is a different number, so this silently writes wrong data — with no warning even in strict mode, while the "C"/"M" path a few lines below raisesDbfStringDataLoss:Also
N(size=2)+-999→b"-9",N(size=1)+-5→b"-"(reads back as None), andN(size=6, decimal=2)+12345.67→b"12345.", a bare decimal point in a field whose decimal count says 2.Both numeric sub-paths (integer and fixed point) now go through one
_pack_dbf_number, which warns, or raises the newDbfNumericDataLossin strict mode, like the text path does. The bytes written are unchanged, so callers who are happy with the truncation just get a warning.DbfStringDataLossandDbfNumericDataLossshare a newDbfDataLossbase (#146), soexcept DbfStringDataLossis unaffected.I checked the rest of the record writer for the same class before fixing it: "D" and "L" can't truncate because
Field.from_uncheckedforces their size to 8 and 1, and "C"/"M" already signal, so it really is just those two lines. Tests cover both sub-paths, signed and unsigned, integer and decimal, the fields that fit (no warning), and the D/L immunity.tests/hypothesis_tests.pyneededexcept shp.DbfStringDataLosswidened toDbfDataLoss: the strategy generatesN(size=9, decimal=1)with9999999.96875, which rounds to"10000000.0"and was being stored asb"10000000.". The round trip passed because the expected value is rounded the same way, so it matched the corrupted read-back.One thing I left alone as it's a design call rather than a bug fix:
float("inf")andfloat("nan")fit an 8 wide field and get written asb" inf"/b" nan", which aren't dbf numeric literals. Happy to follow up if you'd want NaN treated as MISSING.Changelog is under 3.1.7 — move it if you'd rather have it elsewhere. Ran
ruff check,ruff format,mypy --strict, pytest (170 passed),-m hypothesis(18),-m network(1 passed, 1 skipped), andtests/run_doctests.py(231).