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 dictionary deserializer #189

Merged
merged 6 commits into from
Feb 27, 2019
Merged

Fix dictionary deserializer #189

merged 6 commits into from
Feb 27, 2019

Conversation

ousttrue
Copy link
Contributor

  • JsonDictionaryValidator.Deserialize is not implemented
  • Fix dictionary member serialization

@ousttrue ousttrue mentioned this pull request Feb 12, 2019
@ousttrue ousttrue added this to the v0.52.0 milestone Feb 13, 2019
@@ -219,10 +219,32 @@ public void HumanoidTest()
[Test]
public void MaterialTest()
{
var model = new glTF_VRM_Material();
var model = new glTF_VRM_Material
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add this test case as a new unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple case is 0eff571#diff-1967e80b396b0aed6d6a20cb02357f56R162

Addition of MaterialTest is extra.

Copy link
Contributor

@yutopp yutopp Feb 15, 2019

Choose a reason for hiding this comment

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

I see. It is OK if you can say that the test case previously checked is not affected by changes in this PR.

// deserialize
var deserialized = default(glTF_VRM_Material);
json.ParseAsJson().Deserialize(ref deserialized);
Assert.AreEqual(1, deserialized.floatProperties.Count);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that element values should be checked not only checking number of elements. Same comments for other fields.

DeserializeValue(HogeFuga.Fuga, "1");

DeserializeValue(new EnumTest(), "{\"EnumDefault\":0,\"EnumAsInt\":0,\"EnumAsString\":\"Hoge\",\"EnumAsLowerString\":\"hoge\"}");
DeserializeValue(new DictionaryValue(), "{\"Dict\": {}}");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple case is here

@@ -219,10 +219,32 @@ public void HumanoidTest()
[Test]
public void MaterialTest()
{
var model = new glTF_VRM_Material();
var model = new glTF_VRM_Material
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple case is 0eff571#diff-1967e80b396b0aed6d6a20cb02357f56R162

Addition of MaterialTest is extra.

@ousttrue
Copy link
Contributor Author

以後、日本語で。
特に直す必要無いと思います。
マージしたい。

@ousttrue ousttrue merged commit bedf655 into master Feb 27, 2019
@yutopp yutopp deleted the fix_dictionary_deserializer branch March 6, 2019 10:15
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.

2 participants