fix: parsing numbers more than i32 and u32 #87
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
is there anything i can do to help this PR along? |
tustvold
approved these changes
Jan 3, 2023
There was a problem hiding this comment.
Apologies this slipped off my radar somehow, just some minor nits.
Could I also ask you to please sign the CLA and I can then get this in.
Edit: looking into CI failures (CI should run and work if you push another commit)
pbjson-types/src/value.rs
Outdated
| @@ -152,14 +152,20 @@ impl<'de> serde::de::Visitor<'de> for KindVisitor { | |||
| where | |||
| E: de::Error, | |||
| { | |||
| self.visit_i32(v.try_into().map_err(de::Error::custom)?) | |||
| if v > -(1 << 53) && v < 1 << 53 { | |||
There was a problem hiding this comment.
Suggested change
| if v > -(1 << 53) && v < 1 << 53 { | |
| if v > -(1 << f64::MANTISSA_DIGITS) && v < 1 << f64::MANTISSA_DIGITS { |
pbjson-types/src/value.rs
Outdated
| @@ -187,14 +193,20 @@ impl<'de> serde::de::Visitor<'de> for KindVisitor { | |||
| where | |||
| E: de::Error, | |||
| { | |||
| self.visit_u32(v.try_into().map_err(de::Error::custom)?) | |||
| if v < 1 << 53 { | |||
There was a problem hiding this comment.
Suggested change
| if v < 1 << 53 { | |
| if v < 1 << f64::MANTISSA_DIGITS { |
done |
|
Sorry this took quite so long to get to... I've been a bit distracted with other projects 😅 |
Farhad-Shabani
added a commit
to informalsystems/pbjson
that referenced
this pull request
Dec 20, 2023
* chore: fix CI builds (influxdata#104) * chore: update prost to 0.12 (influxdata#111) * chore: update prost to 0.12 * chore: fix lint * chore: update descriptors --------- Co-authored-by: Raphael Taylor-Davies <[email protected]> * fix: parsing numbers more than i32 and u32 (influxdata#87) * fix: parsing numbers more than i32 and u32 * fix: f64::MANTISSA_DIGITS instead of 53 --------- Co-authored-by: Raphael Taylor-Davies <[email protected]> * fix: Resolve Envoy xDS proto issues (influxdata#103) * fix: Escape Self since it is a reserved keyword * fix: Allow duplicate numbers in enums when aliases are enabled * fix: Avoid field name collisions with the map ident * fix: follow cargo best practice in build.rs script (influxdata#90) Using env! inside the build.rs script makes it difficult to build this package with other build tools like bazel which sandbox execution, since the source directory might be at different paths between when the build.rs file is compiled, and when the resulting build script is executed. Cargo recommends that CARGO_MANIFEST_DIR is read at runtime instead: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts The above links also includes the promise that Cargo will set the cwd to the crate's directory. This patch uses this to find the descriptors.bin file. Co-authored-by: Raphael Taylor-Davies <[email protected]> * chore(deps): update itertools requirement from 0.10 to 0.11 (influxdata#98) Updates the requirements on [itertools](https://github.com/rust-itertools/itertools) to permit the latest version. - [Changelog](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md) - [Commits](rust-itertools/itertools@v0.10.0...v0.11.0) --- updated-dependencies: - dependency-name: itertools dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Raphael Taylor-Davies <[email protected]> * chore(deps): update base64 requirement from 0.13 to 0.21 (influxdata#89) * chore(deps): update base64 requirement from 0.13 to 0.21 Updates the requirements on [base64](https://github.com/marshallpierce/rust-base64) to permit the latest version. - [Release notes](https://github.com/marshallpierce/rust-base64/releases) - [Changelog](https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md) - [Commits](marshallpierce/rust-base64@v0.13.0...v0.21.0) --- updated-dependencies: - dependency-name: base64 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * fix: fix removed methods * fix: indifferent padding --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Raphael Taylor-Davies <[email protected]> * chore: prepare 0.6.0 (influxdata#112) * fix: prost deprecations (influxdata#113) * chore: patch release pjson-build 0.6.2 (influxdata#114) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Bob Aman <[email protected]> Co-authored-by: Jeffrey Smith II <[email protected]> Co-authored-by: Raphael Taylor-Davies <[email protected]> Co-authored-by: Nikolay Alexandrov <[email protected]> Co-authored-by: Thomas Gibson-Robinson <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Raphael Taylor-Davies <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix #85