Fix missing support for 128-bit integers#731
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tyranron
force-pushed
the
fix-parsing-into-i128
branch
from
3ca2522 to
f99d86e
Compare
Pull Request Test Coverage Report for Build 21992788757Details
💛 - Coveralls |
tyranron
force-pushed
the
fix-parsing-into-i128
branch
2 times, most recently
from
e06fd8c to
6981823
Compare
|
CI error with failed coverage publishing is not relevant to this PR. Everything else made green. |
src/ser.rs
Outdated
| fn serialize_i128(self, v: i128) -> Result<Self::Ok> { | ||
| if v > i64::MAX.into() { | ||
| Err(ConfigError::Message(format!( | ||
| "value {} is greater than the max {}", | ||
| v, | ||
| i64::MAX | ||
| ))) | ||
| } else if v < i64::MIN.into() { | ||
| Err(ConfigError::Message(format!( | ||
| "value {} is less than the min {}", | ||
| v, | ||
| i64::MIN | ||
| ))) | ||
| } else { | ||
| self.serialize_i64(v as i64) | ||
| } |
There was a problem hiding this comment.
Why are we checking the ranges rather than serializing as a primitive?
There was a problem hiding this comment.
@epage I don't know. I did repeat the logic that was already there for the serialize_u64(). It seems strange to me too, but I can only speculate that it's there for compatibility with JSON integer types, or something.
Personally, I don't see any value in it. But changing this would be a subtle breaking change, so seemed to be out of scope for this PR.
There was a problem hiding this comment.
I fixed the serialize_u64 logic in #732.
If its safe to introduce this code and it is safe to remove error cases (which it generally is), then it seems to be safe to make this change. If you have a concrete concern over how this would break someone, I would be interested in hearing it.
There was a problem hiding this comment.
I fixed the
serialize_u64logic in #732.
Nice! I've updated the code and the tests.
If you have a concrete concern over how this would break someone, I would be interested in hearing it.
As I've said before, only speculations so far:
That check was there for some reason. So maybe it was added to disallow serializing configs, which couldn't be parsed by other languages (such as JS). And so, maybe some code using the config crate may rely on that hidden contract.
Since this is a hidden behavior change that may, probably, cause some breaks, I think it should be documented very well in the CHANGELOG, and the crate version bumped to 0.16.0. Just to be safe.
tyranron
force-pushed
the
fix-parsing-into-i128
branch
from
8e0d092 to
af36c24
Compare
tyranron
force-pushed
the
fix-parsing-into-i128
branch
from
af36c24 to
0593ae2
Compare
|
@epage done. |
3 participants
Complements #178
Related to #161
Synopsis
At the moment trying to deserialize
Configinto a struct withu128/i128errors:This happens even if the parsed value is small and fits into other integers.
Solution
This PR wires the
Value'si128/u128functionality intoDeserializer/Serializerimplementations.