search

davidhewitt / pythonize

MIT License
207 stars 28 forks source link

Add tests for deserializing unsigned integers #67

Closed Walter-Reactor closed 4 months ago

Walter-Reactor commented 4 months ago

Adds unit tests demonstrating correct behavior when deserializing u64 & i64

This could cause problems if the integer was larger than could be represented by an i64.

With this change, any positive value will be preferentially deserialized as a u64, only falling back to an i64 if extraction as a u64 fails.

Also introduces a panic if the value is not representable with either of these, which while perhaps not ideal is better than a silent failure IMO

MarshalX commented 4 months ago

IMO it will be awesome to cover this case with tests

davidhewitt commented 4 months ago

Thanks for the PR! Agreed a test would be great, and I am not comfortable with the panic. We should error on overflow (and a test for that would be fantastic).

Walter-Reactor commented 4 months ago

Upon adding tests I discovered that this works just fine as is, so I'm editing the PR to just add unit tests.

Sorry for the alarm! Someone had made this edit on an internal fork so I assumed it was necessary.

davidhewitt commented 4 months ago

Thanks @Walter-Reactor - I think actually you might have been onto something, I did some experimentation and have now pushed #69 which I think fixes some edge cases with large numbers. I'll close this one, but thank you for reporting / pushing this first pass!