Adds support for fields and null values #2

skigrinder · 2020-05-17T14:11:27Z

Adds support for fields and null values which fail in the test application

Motivation and Context

Required to ensure serialization and deserialization work correctly.

How Has This Been Tested?

Against the included test program.

Types of changes

Improvement (non-breaking change that improves a feature, code or algorithm)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Add files via upload

nfbot · 2020-05-17T14:11:28Z

Hi @skigrinder,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

CLAassistant · 2020-05-17T14:11:31Z

All committers have signed the CLA.

networkfusion

Please see the comments. Also, why have you created a new project and not used the sample one?

networkfusion · 2020-05-17T14:21:42Z

source/nanoFramework.Json.Test/Program.cs

@@ -55,7 +55,7 @@ public static void Main()

            DoArrayTest();
            DoSimpleObjectTest();
-            DoFloatNaNObjectTest();
+            //DoFloatNaNObjectTest();


Lets keep the test, even if it fails, so that people can see...

Didn't you create the Json.Test project?

Sorry, the json nfproj seems to be totally recreated in your PR (not the test one)... not sure why that is...

I think it probably has to do with the nerdbank NuGet. I had updated to the latest beta version and didn't realized this would cause problems. I tried to revert back to the stable version and things got really messed up.

Figured out the issues with Json.Test - it now builds

source/nanoFramework.Json.Test/nanoFramework.Json.Test.nfproj

networkfusion · 2020-05-17T14:24:52Z

source/nanoFramework.Json/DebugHelper.cs

@@ -13,7 +16,7 @@ internal static class DebugHelper
        // Used this a lot in the effort to get Array serialization & deserialization working
        public static void DisplayDebug(string displayString)
        {
-#if DEBUG
+#if NANOFRAMEWORK_DISPLAY_DEBUG


nanoFramework will automatically show the Console.WriteLine when #if DEBUG is used and the lib is in debug mode, there is no reason for NANOFRAMEWORK_DISPLAY_DEBUG here. Please revert. (note, it is now preferable to use Debug.WriteLine which removes the need for this function entirely)

I thought about changing everything to Debug.WriteLine, but there's a lot of output.
Don't most people work in Debug mode?
If so, the serialize/deserialize output would be annoying.
I thought NANOFRAMEWORK_DISPLAY_DEBUG would be a convenient way to turn on the lib output if we ever need to work on it again.

Not sure how Debug works when referencing published nugets… @josesimoes will Debug be output when published and referenced?

networkfusion · 2020-05-17T14:25:47Z

source/nanoFramework.Json/JsonConvert.cs

 								{
 									dt = DateTimeExtensions.FromASPNetAjax(sdt);
-								}
-								else
+								} 


Please try and remove any whitespace changes, they make reviewing a lot harder

Sorry, this was an attempt to conform to the coding standard.

networkfusion · 2020-05-17T14:26:49Z

source/nanoFramework.Json/JsonConvert.cs

@@ -397,15 +464,15 @@ private static JsonToken Deserialize()
 				throw new Exception("unexpected lexical token during json parse");
 			}
 			return result;
-		}
+		}	// end of Deserialize()


No need for comments like this, as VS can easily shrink functions and this does not conform to the C# nF code style

networkfusion · 2020-05-17T14:27:16Z

source/nanoFramework.Json/JsonConvert.cs

@@ -464,7 +531,7 @@ private static JsonArrayAttribute ParseArray(ref LexToken token)
 			}
 			var result = new JsonArrayAttribute((JsonToken[])list.ToArray(typeof(JsonToken)));
 			return result;
-		}
+		}   // end of ParseArray()


No need for comments like this, as VS can easily shrink functions and this does not conform to the C# nF code style

networkfusion · 2020-05-17T14:27:25Z

source/nanoFramework.Json/JsonConvert.cs

@@ -520,7 +587,7 @@ private static JsonToken ParseValue(ref LexToken token)
 			}

 			throw new Exception("invalid value found during json parse");
-		}
+		}   // end of ParseValue()


No need for comments like this, as VS can easily shrink functions and this does not conform to the C# nF code style

networkfusion · 2020-05-17T14:29:30Z

source/nanoFramework.Json/nanoFramework.Json.nfproj

@@ -72,11 +77,11 @@
      <ProjectConfigurationsDeclaredAsItems />
    </ProjectCapabilities>
  </ProjectExtensions>
-  <Import Project="..\packages\Nerdbank.GitVersioning.3.1.91\build\Nerdbank.GitVersioning.targets" Condition="Exists('..\packages\Nerdbank.GitVersioning.3.1.91\build\Nerdbank.GitVersioning.targets')" />
+  <Import Project="..\packages\Nerdbank.GitVersioning.3.2.5-beta\build\Nerdbank.GitVersioning.targets" Condition="Exists('..\packages\Nerdbank.GitVersioning.3.2.5-beta\build\Nerdbank.GitVersioning.targets')" />


Please dont update this nuget, we need the stable one for the release pipeline

Interesting. This created some trouble for me. I'll revert back to 3.1.91

networkfusion · 2020-05-17T14:29:47Z

source/nanoFramework.Json/nanoFramework.Json.nfproj

  <Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
    <PropertyGroup>
      <ErrorText>This project references NuGet package(s) that are missing on this computer. Enable NuGet Package Restore to download them.  For more information, see http://go.microsoft.com/fwlink/?LinkID=322105.The missing file is {0}.</ErrorText>
    </PropertyGroup>
-    <Error Condition="!Exists('..\packages\Nerdbank.GitVersioning.3.1.91\build\Nerdbank.GitVersioning.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Nerdbank.GitVersioning.3.1.91\build\Nerdbank.GitVersioning.targets'))" />
+    <Error Condition="!Exists('..\packages\Nerdbank.GitVersioning.3.2.5-beta\build\Nerdbank.GitVersioning.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Nerdbank.GitVersioning.3.2.5-beta\build\Nerdbank.GitVersioning.targets'))" />


Please dont update this nuget, we need the stable one for the release pipeline

networkfusion · 2020-05-17T14:29:58Z

source/nanoFramework.Json/packages.config

  <package id="nanoFramework.System.Collections" version="1.0.1-preview.6" targetFramework="netnanoframework10" />
  <package id="nanoFramework.System.Text" version="1.0.0-preview.16" targetFramework="netnanoframework10" />
  <package id="nanoFramework.Windows.Storage.Streams" version="1.8.2-preview.5" targetFramework="netnanoframework10" />
-  <package id="Nerdbank.GitVersioning" version="3.1.91" targetFramework="netnanoframework10" developmentDependency="true" />
+  <package id="Nerdbank.GitVersioning" version="3.2.5-beta" targetFramework="netnanoframework10" developmentDependency="true" />


Please dont update this nuget, we need the stable one for the release pipeline

networkfusion · 2020-05-21T00:15:13Z

source/nanoFramework.Json/JsonConvert.cs

 						else if (memberProperty.Value is JsonValue)
 						{
 							// Don't need any more info - populate the member using memberSetMethod.Invoke()
 							DebugHelper.DisplayDebug($"{debugIndent}     memberProperty.Value is JValue");
-							if (memberType != typeof(DateTime))
+							if (memberType != typeof(DateTime)) 


Extra space?!

networkfusion · 2020-05-21T00:16:07Z

source/nanoFramework.Json/JsonConvert.cs

 							{
 								DateTime dt;
 								var sdt = ((JsonValue)memberProperty.Value).Value.ToString();
-								if (sdt.Contains("Date("))
+								if (sdt.Contains("Date(")) 


Extra space?!

networkfusion · 2020-05-21T00:16:37Z

source/nanoFramework.Json/JsonConvert.cs

 							}
-						}
+						} 


Extra space?!

networkfusion · 2020-05-21T00:17:05Z

source/nanoFramework.Json/JsonConvert.cs

@@ -436,13 +503,13 @@ private static JsonObjectAttribute ParseObject(ref LexToken token)
 				throw new Exception("unterminated json object");
 			}
 			return result;
-		}
+		}   // end of ParseObject()


Remove comment

networkfusion · 2020-05-21T23:20:28Z

@skigrinder , keep pushing to your branch, the changes will make the comments outdated (due to your changes) so that we are automatically aware...

Fixes some style issues Reverts debug def (as not important for most users)

networkfusion · 2020-05-25T16:55:09Z

@skigrinder , I hope you dont mind, but I have cleaned up the rest of the remaining issues... I just need to test it now, and then it can be committed. Hopefully the commits I have made will help you with future PR's. The main (remaining) issue was that you had created seperate solutions for each project, when you should have just opened the existing one...

networkfusion · 2020-05-25T16:58:04Z

In regards to the debug helper, (as confirmed by @josesimoes (in private cononversation)) nugets are only published as "release", so it is wise not to use a custom debug def...

skigrinder · 2020-05-25T20:24:06Z

@networkfusion, thanks for the effort.
I thought I had cleaned up all the issues we talked about.
Did they not get pushed up or did I miss some?

Glad to know that the NuGets are all built as 'Release'.
Should I get rid of the debug helper in favor of Debug.WriteLine() ?
This would make the code cleaner but I don't want to hold up the show.

networkfusion · 2020-05-25T21:08:02Z

@skigrinder not to worry, it didn't take long to do the last little bits, and we are glad for the contribution. plus, you enjoy the fact that the PR was of your making... Yes, feel free to convert to Debug.WriteLine in another PR (using another branch).

skigrinder · 2020-05-25T21:35:38Z

Hi Robin, Will my existing fork eventually get rolled in and disappear? If so, I don't want to cause confusion by creating another fork (or is that OK?). Please let me know. Thanks, Jim

…

On 2020-05-25 3:08 PM, Robin Jones wrote: @skigrinder <https://github.com/skigrinder> not to worry, it didn't take long to do the last little bits, and we are glad for the contribution. plus, you enjoy the fact that the PR was of your making... Yes, feel free to convert to |Debug.WriteLine| in another PR (using another branch). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO2PVOMJHO3QB4MAROCNGELRTLM35ANCNFSM4NDM3NWQ>.

networkfusion · 2020-05-25T21:43:49Z

Since you didn't use a different branch, you will need to do something like https://ardalis.com/syncing-a-fork-of-a-github-repository-with-upstream (although on the develop branch instead of master. The althernative is to "delete" your fork and start again (it will cause no issues now that this PR is closed). I have done both in the past... But for next time, make sure you create a new branch when making changes in your own fork, and then you can just switch to develop pull the changes and merge onto your new branch whenever nF changes... there is lots of info out there, just google it...

skigrinder · 2020-05-26T02:47:21Z

Hi Robin, I blew away my original fork and created a new one. This created a new copy of the current lib in a new repository on my GitHub. In my new repository, I created a new branch - "skigrinder - use Debug.WriteLine()" I tried to create a new branch in the nanoFramework repository but it wouldn't let me. Doesn't this need to be done? I don't want to go any further for fear of making the same mistake as last time. I've done a lot of googling and it seems like there should be a new branch on the nanoFramework repository before I start making & commiting changes (but I can't create one). Or, will a new branch get created on the nanoFramework repository when I do the PR? Please let me know if I'm on the right path or if I should clobber my new fork and start over. Thanks, Jim

…

On 2020-05-25 3:44 PM, Robin Jones wrote: Since you didn't use a different branch, you will need to do something like https://ardalis.com/syncing-a-fork-of-a-github-repository-with-upstream (although on the |develop| branch instead of |master|. The althernative is to "delete" your fork and start again (it will cause no issues now that this PR is closed). I have done both in the past... But for next time, make sure you create a new branch when making changes in your own fork, and then you can just switch to |develop| pull the changes and merge onto your new branch whenever nF changes... there is lots of info out there, just google it... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO2PVOP4LCJI2ZFQ4MT5TQ3RTLRCDANCNFSM4NDM3NWQ>.

networkfusion · 2020-05-26T03:51:14Z

You need to create the branch in your fork. The branch will be available to nF when a PR is raised. At that point, the branch will be merged into this repos develop branch...

skigrinder · 2020-05-26T12:40:52Z

Got it & thanks! I was hoping that's how it works but wanted to make sure. Thanks, Jim

…

On 2020-05-25 9:51 PM, Robin Jones wrote: You need to create the branch in your fork. The branch will be available to nF when a PR is raised. At that point, the branch will be merged into this repos develop branch... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO2PVOLK3XDXNQMUPCIUFXDRTM4D5ANCNFSM4NDM3NWQ>.

skigrinder · 2020-05-26T21:02:19Z

Hi Robin, Just finished making the Debug.WriteLine() changes had have done a PR. Please let me know if you can see it and if I need to do anything more. Thanks again for all your help. Every time I do this it gets a little easier. Thanks, Jim

…

On 2020-05-25 9:51 PM, Robin Jones wrote: You need to create the branch in your fork. The branch will be available to nF when a PR is raised. At that point, the branch will be merged into this repos develop branch... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO2PVOLK3XDXNQMUPCIUFXDRTM4D5ANCNFSM4NDM3NWQ>.

skigrinder added 10 commits May 16, 2020 18:43

Add files via upload

b4181ae

Merge pull request #1 from skigrinder/skigrinder-patch-1

f5b9f11

Add files via upload

Delete DebugHelper.cs

debdc84

Delete JsonArrayAttribute.cs

4123f26

Delete JsonConvert.cs

a4f3733

Delete JsonObjectAttribute.cs

57bdd3b

Delete JsonPropertyAttribute.cs

084a2a6

Delete JsonToken.cs

0de45d4

Delete JsonValue.cs

397dfa9

skigrinder - changes to support fields and null values

b9a5463

networkfusion reviewed May 17, 2020

View reviewed changes

networkfusion reviewed May 21, 2020

View reviewed changes

networkfusion changed the title ~~skigrinder - added support for fields and null values~~ Adds support for fields and null values May 21, 2020

Trying to get the solution to build - 001

5cb8c49

josesimoes added the Type: Bug Something isn't working label May 22, 2020

skigrinder added 5 commits May 22, 2020 16:34

nanoFramework.Json solution now builds properly

ea4ebef

Use Runtime.Native - i.e. not Runtime.Events

6974ebd

Json.Test project fixes

0e7cd7a

Fixed Json.Test - now references Json project instead of .dll

f41b630

Removed Collections NuGet from Json.Test

f52afc8

josesimoes requested a review from networkfusion May 23, 2020 19:49

networkfusion added 3 commits May 25, 2020 17:29

Reverts solution changes.

41b1f7b

Fixes some style issues Reverts debug def (as not important for most users)

Remove nested solutions

8d4271c

Remove un-necessary comments

f0dc2ed

networkfusion merged commit e3d7953 into nanoframework:develop May 25, 2020

Adds support for fields and null values #2

Adds support for fields and null values #2

Conversation

skigrinder commented May 17, 2020 • edited by networkfusion Loading

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

nfbot commented May 17, 2020

CLAassistant commented May 17, 2020 • edited Loading

networkfusion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

networkfusion May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skigrinder May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

networkfusion commented May 21, 2020

networkfusion commented May 25, 2020

networkfusion commented May 25, 2020

skigrinder commented May 25, 2020

networkfusion commented May 25, 2020

skigrinder commented May 25, 2020 via email

networkfusion commented May 25, 2020

skigrinder commented May 26, 2020 via email

networkfusion commented May 26, 2020

skigrinder commented May 26, 2020 via email

skigrinder commented May 26, 2020 via email

skigrinder commented May 17, 2020 •

edited by networkfusion

Loading

CLAassistant commented May 17, 2020 •

edited

Loading

networkfusion May 21, 2020 •

edited

Loading

skigrinder May 17, 2020 •

edited

Loading