Regression in SimpleJson 0.32.0

[haacked]

I tried to upgrade to 0.32.0 but found a regression.

Check out the unit test in Octokit.net here: https://github.com/octokit/octokit.net/blob/master/Octokit.Tests/Models/CommitTests.cs#L11-L71

That used to pass with 0.30.0. But with 0.32.0 it fails.

CommitTests+Serialization.PerformsCommitSerialization [FAIL] Assert.Equal() Failure Position: First difference is at position 363 Expected: {"message":"commit-message","author":{"name":"author-name","email":"author-email","date":"2013-10-15T13:40:14Z"},"committer":{"name":"committer-name","email":"committer-email","date":"2013-06-29T10:12:50Z"},"tree":{"url":"tree-url","sha":"tree-reference"},"parents":[{"url":"parent1-url","sha":"parent1-reference"},{"url":"parent2-url","sha":"parent2-reference"}],"url":"commit-url","sha":"commit-reference"} Actual: {"message":"commit-message","author":{"name":"author-name","email":"author-email","date":"2013-10-15T13:40:14Z"},"committer":{"name":"committer-name","email":"committer-email","date":"2013-06-29T10:12:50Z"},"tree":{"url":"tree-url","sha":"tree-reference"},"parents":[{"url":"parent1-url","sha":"parent1-reference"},{"url":"parent2-url","sha":"parent2-reference"}]} Stack Trace: c:\Users\half-ogre\GitHub\qed\qed\bin\Release.repositories\octokit\octokit.net\Octokit.Tests\Models\CommitTests.cs(70,0): at CommitTests.Serialization.PerformsCommitSerialization()

It seems to have ommitted the Sha and Url properties on Commit itself.

[prabirshrestha]

The only thing that was added in 0.32.0 was SIMPLE_JSON_READONLY_COLLECTIONS. Is the test failing when that is enabled or disabled?

[haacked]

OH! I wonder if we should have OR'd that with NET_45. Because folks upgrading won't have this defined in their projects.

[haacked]

So my proposal is on line 1655:

#if SIMPLE_JSON_READONLY_COLLECTIONS || NET_45

If you agree, I can send a PR.

[prabirshrestha]

@Haacked seems like this only affects metro apps (haven't tried the phones yet).

Creating a brand new .net 4.5 project and running the test does pass, but creating a metro app and running doesn't pass. Seems like the xunit is running the metro build and thus failing.

will look into it later tonight.

[prabirshrestha]

We currently don't use the flag NET_45. We use feature test rather then framework version test like in javascript. Test for data contact (SIMPLE_JSON_DATACONTRACT), type info (SIMPLE_JSON_TYPEINFO), linq expressions (SIMPLE_JSON_NO_LINQ_EXPRESSION). This allows to support new .net versions/fwk very easily.

The only exception we have is with NETFX_CORE which is converted to feature test. And this the only place where NETFX_CORE is used.

#if NETFX_CORE
#define SIMPLE_JSON_TYPEINFO
#endif

#if NET_45
#define SIMPLE_JSON_READONLY_COLLECTIONS
#endif

But is NET_45 use by lot of projects in order to consider it as a standard?

I think I found the problem. The reason Sha and Url was omitted is coz it is in base class.

The below line gives 5 for metro apps (doesn't include Sha and Url) and 12 for net 4.5 apps (includes Sha and Url). I would consider it as a bug in reflection utils.

var x = ReflectionUtils.GetProperties(typeof(Commit)).ToList();
Debug.WriteLine(x.Count);

If you do send a PR, make sure to send the reflection-utils fix in https://github.com/facebook-csharp-sdk/reflection-utils too.

public static IEnumerable<PropertyInfo> GetProperties(Type type)
{
#if SIMPLE_JSON_TYPEINFO
    return type.GetTypeInfo().DeclaredProperties;
#else
    return type.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static);
#endif
}

[prabirshrestha]

@Haacked can you test octokit.net with this patch https://github.com/facebook-csharp-sdk/simple-json/pull/47

[haacked]

Seems to work

On Thu, Feb 27, 2014 at 4:07 PM, Prabir Shrestha notifications@github.comwrote:

@Haacked https://github.com/Haacked can you test ocktokit.net with this patch #47 https://github.com/facebook-csharp-sdk/simple-json/pull/47

— Reply to this email directly or view it on GitHubhttps://github.com/facebook-csharp-sdk/simple-json/issues/46#issuecomment-36307853 .

[prabirshrestha]

fixed in v0.36.0 by #49