-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Neo Json Fix] Json null tests #3450
Conversation
@@ -252,7 +252,7 @@ public void TestAsString() | |||
bob, | |||
}; | |||
var s = jArray.AsString(); | |||
Assert.AreEqual(s, "{\"name\":\"alice\",\"age\":30,\"score\":100.001,\"gender\":\"female\",\"isMarried\":true,\"pet\":{\"name\":\"Tom\",\"type\":\"cat\"}},{\"name\":\"bob\",\"age\":100000,\"score\":0.001,\"gender\":\"male\",\"isMarried\":false,\"pet\":{\"name\":\"Paul\",\"type\":\"dog\"}}"); | |||
Assert.AreEqual(s, "[{\"name\":\"alice\",\"age\":30,\"score\":100.001,\"gender\":\"female\",\"isMarried\":true,\"pet\":{\"name\":\"Tom\",\"type\":\"cat\"}},{\"name\":\"bob\",\"age\":100000,\"score\":0.001,\"gender\":\"male\",\"isMarried\":false,\"pet\":{\"name\":\"Paul\",\"type\":\"dog\"}}]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it change the behavior, and it could require a hardfork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And JSON is available from contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it must be a hardfork, then make it a hardfork, this one also need to be reviewed:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change the contact behavior is bad,,,,,,,i know, but what we gonna do with tons of problems in the core....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And JSON is available from contracts.
BTW, how does contract use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And without []
is only part of the problem, it also lacks null
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Than what is the point of having AsString
vs ToString
? If they both do the samething.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Than what is the point of having
AsString
vsToString
? If they both do the samething.
To be honest i dont know,,,, and there is no document for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And without
[]
is only part of the problem, it also lacksnull
object.
could you fix both at same time, but we need to ensure that is not possible to call it by a smart contract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And without
[]
is only part of the problem, it also lacksnull
object.could you fix both at same time, but we need to ensure that is not possible to call it by a smart contract
Already fixed.
change branch to HF? |
No, this one does not require HF. It is not being used actually. |
Description
Json JArry has a method
AsString
but it can not properly convert json into json string, but a string without "[ ]",And it can not handle null value. Thus having this pr that fix this bug and adds more tests.
Fixes #
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: