Skip to content

Conversation

@shervinw
Copy link
Contributor

@shervinw shervinw commented Sep 5, 2019

Based from the original null Values fix attempt from @nickevansuk, this PR just adds minor changes to solve the problem. Also see the problem origin.

Json.net has an internal method Newtonsoft.Json.Utilities.MiscellaneousUtils.ValueEquals(object objA, object objB) which utilises IEquatable to determine whether or not to write a property when DefaultValueHandling is specified.

The bulk of this fix occurs in Values{T1,T2}, Values{T1,T2, T3}, Values{T1,T2, T3, T4}, which contain method overrides specific to the issue.

I have also added an additional test that was previously removed.

@shervinw
Copy link
Contributor Author

shervinw commented Sep 5, 2019

Also to note, running the generator causes the build to fail due to incompatible types, manually removed the following extract from Thing.cs to match master.

/// <summary>
/// Gets the context for the object, specifying that it comes from schema.org.
/// </summary>
[DataMember(Name = "@context", Order = 0)]
public override string Context => "http://schema.org";

The generator also modifies a bunch of properties to be of type Values<T> rather than OneOrMany<T>.

@RehanSaeed
Copy link
Owner

Thanks @shervinw for contributing! Have not forgotten about this PR. I'll try to take a look early next week.

@nickevansuk
Copy link
Contributor

This looks exciting @shervinw! @RehanSaeed what do you think?

@RehanSaeed RehanSaeed merged commit e4be377 into RehanSaeed:master Sep 26, 2019
@RehanSaeed
Copy link
Owner

I now have time to take a look at this. I've just upgraded the project to use .NET Core 3.0 and also publish NuGet packages to the GitHub package registry.

Bravo! A simple change that seems to solve all issues. Not sure why I missed it.

Added you as a contributor on the main page as thanks.

@RehanSaeed
Copy link
Owner

Will release a major version update shortly.

@nickevansuk
Copy link
Contributor

Great news guys! Well done @shervinw !

@RehanSaeed can I check that this lib will still be compatible with .NET Core 1 and 2? As our library built on top of it needs to be backwards compatible too...

@RehanSaeed
Copy link
Owner

Yes it will. We still target netstandard1.1 and netstandard2.0. I only use .NET Core 3 SDK to build, test and pack the package. It also gives us HTML test results.

@shervinw
Copy link
Contributor Author

shervinw commented Sep 26, 2019 via email

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