Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Issue #202 and #190 #206

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

quetzyg
Copy link
Contributor

@quetzyg quetzyg commented Apr 5, 2021

Description

This pull request aims to fix numeric precision when decoding, fixing issue #202 and potentially #190 and #120?

Task items:

  • Preserve the literal value of a numeric type when unmarshalling/decoding via UseNumber();
  • Minor update to a test to prove the precision is kept;
  • Add coverage for the all numeric types (handleNumeric() function);

@@ -303,7 +303,7 @@ func TestUnmarshalSetsID(t *testing.T) {
t.Fatal(err)
}

if out.ID != 2 {
if out.ID != 9223372036854775807 {
Copy link
Contributor Author

@quetzyg quetzyg Apr 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we used this value previously, the test would fail;

request.go Outdated Show resolved Hide resolved
@quetzyg
Copy link
Contributor Author

quetzyg commented Apr 5, 2021

@aren55555 lmk what you think.

@@ -495,27 +498,26 @@ func handleTime(attribute interface{}, args []string, fieldValue reflect.Value)
return reflect.ValueOf(t), nil
}

var at int64
switch v.Interface().(type) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original logic was kept, since we're still checking the type of the attribute, while the previous/following checks are done to the fieldValue kind.

I wonder if we should check the fieldValue kind here as well for consistency.

request.go Outdated Show resolved Hide resolved
@quetzyg quetzyg marked this pull request as ready for review April 5, 2021 21:14
@aren55555
Copy link
Contributor

I'll take a look on Sunday when I'll actually be able to allocate enough time to dig into this.

@quetzyg
Copy link
Contributor Author

quetzyg commented Apr 12, 2021

@aren55555 the work on this PR is done, do take a look.

@wurmrobert
Copy link

wurmrobert commented May 19, 2021

@aren55555 would be nice if you can merge according the dependency to #122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants