Fix #57, fix #58: Emit initializers and attributes for default values #60

ghost · 2018-12-28T20:09:29Z

#57: When a schema property declares a default value, assign that value to the property in the default constructor. This ensures that the property has the correct default even if you construct the object by hand rather than by deserializing it from JSON.

#58: When a schema property declares a default value, decorate the property with a System.ComponentModel.DefaultValue attribute. This avoids your having to specify those attributes in the CodeGenHints.json file, which is error-prone.

Also:

Fix a bug where the generated doc comments for properties had an extra space: cref="P: propName" → cref="P:propName".
Address some hygiene-related IDE messages by marking some properties readonly, and by inlining the declarations of some out parameters.

ghost · 2018-12-28T20:11:32Z

src/Json.Schema.ToDotNet/Json.Schema.ToDotNet.csproj

+      <Generator>ResXFileCodeGenerator</Generator>
+      <LastGenOutput>Resources.Designer.cs</LastGenOutput>
+    </EmbeddedResource>
+  </ItemGroup>


I omitted these elements when I converted JSchema to .NET Core. Without them, the .Designer.cs file isn't regenerated when you edit the .resx file. #ByDesign

ghost · 2018-12-28T20:12:37Z

// Copyright (c) Microsoft Corporation. All Rights Reserved.

All changes in this file are hygiene-related.

Refers to: src/Json.Schema.ToDotNet/PropertyInfoDictionary.cs:1 in 6ef6d4d. [](commit_id = 6ef6d4d, deletion_comment = False)

ghost · 2018-12-28T20:13:39Z

src/Json.Schema.ToDotNet/Resources.resx

@@ -169,7 +169,7 @@
    <value>An uninitialized kind.</value>
  </data>
  <data name="PropertyCtorParamDescription" xml:space="preserve">
-    <value>An initialization value for the &lt;see cref="P: {0}" /&gt; property.</value>


Fix minor existing bug in generation of doc comments for properties. #Closed

ghost · 2018-12-28T20:14:37Z

src/Json.Schema.ToDotNet/TypeGenerator.cs

-            if (Usings == null)
+            Usings = Usings ?? new List<string>();
+
+            if (!Usings.Contains(namespaceName))


Contains [](start = 24, length = 8)

Avoid duplicates in this list. This has never been a problem; there's probably code somewhere else that de-dupes it, but I can't be troubled to look for it now. #Closed

I found the code for this in SyntaxNodeExtensions.Format. This test is not needed. Reverting.

In reply to: 244399389 [](ancestors = 244399389)

ghost · 2018-12-28T20:15:11Z

src/Json.Schema.ToDotNet.UnitTests/DataModelGeneratorTests.cs

@@ -871,10 +871,43 @@ public void GeneratesCloningCode()
      ""type"": ""integer"",
      ""description"": ""An integer property.""
    },
+    ""intPropWithDefault"": {


intPropWithDefault [](start = 6, length = 18)

Add properties with default values for all JSON schema primitive types.

ghost · 2018-12-28T20:15:34Z

src/Json.Schema.ToDotNet.UnitTests/DataModelGeneratorTests.cs

+      ""default"": 42
+    },
+    ""numberProp"": {
+      ""type"": ""number"",


number [](start = 18, length = 6)

Add coverage for number properties, which we previously lacked.

ghost · 2018-12-28T20:15:50Z

src/Json.Schema.ToDotNet.UnitTests/DataModelGeneratorTests.cs

+      ""description"": ""A string property with a default value."",
+      ""default"": ""42""
+    },
+    ""boolProp"": {


boolProp [](start = 6, length = 8)

We didn't cover bool properties before, either.

ghost · 2018-12-28T20:16:33Z

src/Json.Schema.ToDotNet.UnitTests/DataModelGeneratorTests.cs

+            NumberPropWithDefault = 42.1;
+            StringPropWithDefault = ""42"";
+            BoolPropWithTrueDefault = true;
+            BoolPropWithFalseDefault = false;
        }


} [](start = 8, length = 1)

Demonstrates fix for #57.

ghost · 2018-12-28T20:17:05Z

src/Json.Schema.ToDotNet.UnitTests/DataModelGeneratorTests.cs

+        /// An integer property with a default value.
+        /// </summary>
+        [DataMember(Name = ""intPropWithDefault"", IsRequired = false, EmitDefaultValue = false)]
+        [DefaultValue(42)]


[DefaultValue(42)] [](start = 8, length = 18)

Demonstrates fix for #58.

ghost · 2018-12-28T20:18:59Z

src/Json.Schema/JsonSchema.cs

+            if (left is long) { return (long)left == (long)right; }
+            if (left is double) { return (double)left == (double)right; }
+
+            return left == right; // Not good. How do we deal with other types?


Not [](start = 37, length = 3)

I'm open to suggestion here. Maybe left.Equals(right) is better, because at least it doesn't fail for two distinct objects that are value equal. #Resolved

Why aren't you simply using the Object.Equals implementation entirely for this? I see via reference source that it appears to shell out to some native code for the implementation. but it should handle all this type consideration/unboxing to do its thing. All this code used to be authored as C#, which I've reviewed in past contexts.

In reply to: 244400084 [](ancestors = 244400084)

Thanks for that! It didn't even occur to me. Yes, after removing this function and instead calling Object.Equals(Default, other.Default), all the tests still pass.

In reply to: 244415654 [](ancestors = 244415654,244400084)

ghost · 2018-12-28T20:19:39Z

src/Json.Schema.ToDotNet/ClassGenerator.cs

@@ -279,6 +282,24 @@ protected override AttributeSyntax[] GeneratePropertyAttributes(string propertyN

            attributes.Add(dataMemberAttribute);

+            if (defaultValue != null)


if [](start = 12, length = 2)

Implements fix for #58 (DefaultValue attributes).

ghost · 2018-12-28T20:19:54Z

src/Json.Schema.ToDotNet/ClassGenerator.cs

@@ -372,7 +393,7 @@ private ConstructorDeclarationSyntax GenerateDefaultConstructor()
        {
            return SyntaxFactory.ConstructorDeclaration(SuffixedTypeName)
                .AddModifiers(SyntaxFactory.Token(SyntaxKind.PublicKeyword))
-                .AddBodyStatements()
+                .AddBodyStatements(GenerateDefaultInitializations())


GenerateDefaultInitializations [](start = 35, length = 30)

Implements fix for #57.

ghost · 2018-12-28T20:20:28Z

src/Json.Schema.ToDotNet/ClassGenerator.cs

+                // integer properties as Int64 (long), but we generate integer properties
+                // with type int. The extra cast causes Roslyn to emit the literal 42,
+                // which can be assigned to an int, rather than 42L, which cannot. We should
+                // consider changing the code generation to emit longs for integer properties.


. [](start = 93, length = 1)

Michael, your thoughts on this? #Pending

I see. Yes, the problem is ours, JSON/JavaScript number/integer primitives clearly allow for values that exceed C# Int32. If we're going to make this change we should do it soon! It will be an intrusive breaking change. Since we're expanding the type size, the impact should be minimal beyond the literal type change. But it's worrisome to think about the OM signature impact here.

In reply to: 244400469 [](ancestors = 244400469)

Michael argued that .NET pervasively uses int (not long, not even uint) in its APIs -- and that for historical reasons (because VB didn't support "unsigned"). We need to think hard about this, and assess the impact on interoperability with other .NET APIs, before deciding to do this. Won't do it now.

In reply to: 244417379 [](ancestors = 244417379,244400469)

ghost · 2018-12-28T20:20:42Z

@michaelcfanning is added to the review. #Closed

ghost · 2018-12-28T20:20:52Z

@EasyRhinoMSFT is added to the review. #Closed

ghost · 2018-12-28T21:24:34Z

// Copyright (c) Microsoft Corporation. All Rights Reserved.

All changes in this file are hygiene-related.

Refers to: src/Json.Schema.ToDotNet/TypeGenerator.cs:1 in 38d8c45. [](commit_id = 38d8c45, deletion_comment = False)

michaelcfanning · 2018-12-28T22:01:30Z

src/Json.Schema.ToDotNet/ClassGenerator.cs

+                    ExpressionStatementSyntax initializationStatement = GenerateDefaultInitialization(propertyName, defaultValue);
+                    if (initializationStatement != null)
+                    {
+                        initializations.Add(GenerateDefaultInitialization(propertyName, defaultValue));


GenerateDefaultInitialization(propertyName, defaultValue [](start = 44, length = 56)

can't you just provide 'initializationStatement' here as an arg? #Resolved

Oh good heavens. I can't quite believe I did that! Fixed.

In reply to: 244416058 [](ancestors = 244416058)

michaelcfanning · 2018-12-28T22:02:47Z

src/Json.Schema.ToDotNet/ClassGenerator.cs

+                }
+            }
+
+            return initializations.ToArray();


initializations.ToArray(); [](start = 19, length = 26)

why are you taking the memory costs to generate an array? mostly a List is appropriate for places where an array would be (a List can be enumerated, for example). an Array, in fact, is less flexible than a list, as its size isn't mutable. #Pending

Because of the shape of the Roslyn API. The result of this method call will be passed to Roslyns ConstructorDeclarationSyntax.AddBodyStatements, which takes an argument of type params StatementSyntax[]. I'll see if there's a way to add the statements that doesn't require the array. Hang on...

In reply to: 244416245 [](ancestors = 244416245)

This is a much discussed feature gap in C#. See dotnet/csharplang#179 and dotnet/roslyn#36

In reply to: 244418426 [](ancestors = 244418426,244416245)

michaelcfanning · 2018-12-28T22:05:07Z

src/Json.Schema.ToDotNet/ClassGenerator.cs

+
+            if (value is bool)
+            {
+                SyntaxKind literalSyntaxKind = (bool)value == true


== true [](start = 59, length = 7)

you don't actually need the '== true; here, the bool is sufficient to drive the ternary conditional #WontFix

I know, I waffled on that, but I decided to write it this way because of the way it reads in English: "If the value is true, use a TrueLiteralExpression, otherwise use a FalseLiteralExpression. The "== true" reads as "is true" in my head.

In reply to: 244416578 [](ancestors = 244416578)

i don't know what it is but I mostly find that direct comparisons to bool literals compromise rather than improve readability. this one's a corner case.

In reply to: 244417679 [](ancestors = 244417679,244416578)

michaelcfanning · 2018-12-28T22:12:09Z

src/Json.Schema.ToDotNet/ClassOrInterfaceGenerator.cs

@@ -103,7 +103,7 @@ private PropertyDeclarationSyntax CreatePropertyDeclaration(string propertyName)
                .AddModifiers(GeneratePropertyModifiers(propertyName))
                .AddAccessorListAccessors(GeneratePropertyAccessors());

-            AttributeSyntax[] attributes = GeneratePropertyAttributes(propertyName, info.SerializedName, info.IsRequired);
+            AttributeSyntax[] attributes = GeneratePropertyAttributes(propertyName, info.SerializedName, info.IsRequired, info.DefaultValue);


AttributeSyntax[] [](start = 12, length = 17)

I will stop remarking on the point, but the pervasive use of arrays in this code base result in unnecessary memory allocations. #Pending

Yes, as I mentioned in response to your other comment, many Roslyn APIs take a params Thing[]. I'll see if there is another set of Roslyn APIs that use a better pattern.

In reply to: 244417551 [](ancestors = 244417551)

michaelcfanning · 2018-12-28T22:15:38Z

    protected List<string> Usings { get; private set; }

This s/be an IList. Also, why isn't this an IHashSet? #Resolved

Refers to: src/Json.Schema.ToDotNet/TypeGenerator.cs:40 in 38d8c45. [](commit_id = 38d8c45, deletion_comment = False)

michaelcfanning · 2018-12-28T22:17:37Z

src/Json.Schema.ToDotNet.UnitTests/DataModelGeneratorTests.cs

@@ -871,10 +871,43 @@ public void GeneratesCloningCode()
      ""type"": ""integer"",
      ""description"": ""An integer property.""
    },
+    ""intPropWithDefault"": {


intPropWithDefault [](start = 6, length = 18)

I find the jschema test strategy unsatisfying in that there's no clear marker in tests of the real world impact of changes. Perhaps you could add some single practical example of the framework in use that would illustrate more effectively how updates alter code emit. TBD

ghost · 2018-12-28T22:22:03Z

    protected List<string> Usings { get; private set; }

Yes, that would be better. I'll fix it...

In reply to: 450435940 [](ancestors = 450435940)

Refers to: src/Json.Schema.ToDotNet/TypeGenerator.cs:40 in 38d8c45. [](commit_id = 38d8c45, deletion_comment = False)

ghost · 2018-12-28T22:23:21Z

    protected List<string> Usings { get; private set; }

Actually, there doesn't seem to be an IHashSet<T>, just HashSet<T>. I'll use that.

In reply to: 450436604 [](ancestors = 450436604,450435940)

Refers to: src/Json.Schema.ToDotNet/TypeGenerator.cs:40 in 38d8c45. [](commit_id = 38d8c45, deletion_comment = False)

ghost · 2018-12-28T22:34:57Z

    protected List<string> Usings { get; private set; }

Fixed.

In reply to: 450436751 [](ancestors = 450436751,450436604,450435940)

Refers to: src/Json.Schema.ToDotNet/TypeGenerator.cs:40 in 38d8c45. [](commit_id = 38d8c45, deletion_comment = False)

michaelcfanning

* Remove dependency on RuleMessageId (shortly to be deprecated) (#24) * Release 0.57.0 * Todotnet package (#30) * Add utilities to validation and todotnet packages * Update nuget.exe. Add success message for nuspec-driven package creation. * Update signing scripts * Release 0.58.0 (#32) * Address PR feedback from 0.58.0 release (#33) Also: * Build the `develop` branch in AppVeyor. * Add dotnet publish step to build. (#34) Also: - Use `.CommandLine`, not `.Cli` in the command line tool namespaces. * Disable appveyor email notifications (#36) * Update to latest SARIF SDK + JSON.NET downgrade (#38) * Fix ToDotNet.Cli project file so resources can be found. (#40) When you run the ToDotNet command line app, it fails because it can't find the resource stream. Somehow the `.resx` and the `.Resources.designer` files got separated in the project file. Fix the project file to connect the resources back up. * Rewrite top-level README in preparation for documenting each component. (#47) An almost complete rewrite. * Fix #41: Document Json.Pointer (#48) * embed resx resources (#53) * Add unit test to demonstrate interaction of ClassNameHint with other hints (#54) * Start codegen README (#59) * Fix #57, fix #58: Emit initializers and attributes for default values (#60) #57: When a schema property declares a default value, assign that value to the property in the default constructor. This ensures that the property has the correct default even if you construct the object by hand rather than by deserializing it from JSON. #58: When a schema property declares a default value, decorate the property with a `System.ComponentModel.DefaultValue` attribute. This avoids your having to specify those attributes in the CodeGenHints.json file, which is error-prone. Also: - Fix a bug where the generated doc comments for properties had an extra space: `cref="P: propName`" → `cref="P:propName`". - Address some hygiene-related IDE messages by marking some properties `readonly`, and by inlining the declarations of some `out` parameters. * Fix #61: Generate JsonProperty attribute for properties with defaults (#62) * Add final release note in advance of shipping 0.59.0

* Remove dependency on RuleMessageId (shortly to be deprecated) (#24) * Release 0.57.0 * Todotnet package (#30) * Add utilities to validation and todotnet packages * Update nuget.exe. Add success message for nuspec-driven package creation. * Update signing scripts * Release 0.58.0 (#32) * Address PR feedback from 0.58.0 release (#33) Also: * Build the `develop` branch in AppVeyor. * Add dotnet publish step to build. (#34) Also: - Use `.CommandLine`, not `.Cli` in the command line tool namespaces. * Disable appveyor email notifications (#36) * Update to latest SARIF SDK + JSON.NET downgrade (#38) * Fix ToDotNet.Cli project file so resources can be found. (#40) When you run the ToDotNet command line app, it fails because it can't find the resource stream. Somehow the `.resx` and the `.Resources.designer` files got separated in the project file. Fix the project file to connect the resources back up. * Rewrite top-level README in preparation for documenting each component. (#47) An almost complete rewrite. * Fix #41: Document Json.Pointer (#48) * embed resx resources (#53) * Add unit test to demonstrate interaction of ClassNameHint with other hints (#54) * Start codegen README (#59) * Fix #57, fix #58: Emit initializers and attributes for default values (#60) #57: When a schema property declares a default value, assign that value to the property in the default constructor. This ensures that the property has the correct default even if you construct the object by hand rather than by deserializing it from JSON. #58: When a schema property declares a default value, decorate the property with a `System.ComponentModel.DefaultValue` attribute. This avoids your having to specify those attributes in the CodeGenHints.json file, which is error-prone. Also: - Fix a bug where the generated doc comments for properties had an extra space: `cref="P: propName`" → `cref="P:propName`". - Address some hygiene-related IDE messages by marking some properties `readonly`, and by inlining the declarations of some `out` parameters. * Fix #61: Generate JsonProperty attribute for properties with defaults (#62) * Add final release note in advance of shipping 0.59.0 * Update licensing reference to satisfy NuGet improvements in this area. (#65)

Larry Golding added 6 commits December 27, 2018 09:45

Introduce ClassGenerator.GenerateDefaultInitializations (empty).

fef7259

Add Default to PropertyInfo.

df1b3fe

Emit default initializers for integer and string properties.

5747213

Emit default initializers for Boolean properties.

9259ce5

Add initializers for number properties.

2b8cd14

Emit System.ComponentModel.DefaultValue when needed.

6ef6d4d

ghost requested review from michaelcfanning and EasyRhinoMSFT December 28, 2018 20:09

ghost commented Dec 28, 2018

View reviewed changes

Remove unnecessary test.

38d8c45

michaelcfanning reviewed Dec 28, 2018

View reviewed changes

michaelcfanning approved these changes Dec 28, 2018

View reviewed changes

Address feedback from @michaelcfanning.

bf9479d

ghost merged commit 7358e25 into develop Dec 28, 2018

ghost deleted the users/lgolding/issues-57-58-handle-defaults branch December 28, 2018 23:12

This pull request was closed.

		@@ -279,6 +282,24 @@ protected override AttributeSyntax[] GeneratePropertyAttributes(string propertyN

		attributes.Add(dataMemberAttribute);

		if (defaultValue != null)

Fix #57, fix #58: Emit initializers and attributes for default values #60

Fix #57, fix #58: Emit initializers and attributes for default values #60

Conversation

ghost commented Dec 28, 2018

ghost Dec 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost commented Dec 28, 2018

ghost Dec 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Dec 28, 2018 • edited by ghost 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

ghost Dec 28, 2018 • edited by ghost 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

ghost Dec 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 28, 2018

ghost commented Dec 28, 2018

ghost commented Dec 28, 2018

michaelcfanning Dec 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Dec 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Dec 28, 2018

ghost commented Dec 28, 2018

ghost commented Dec 28, 2018

michaelcfanning left a comment

Choose a reason for hiding this comment

ghost Dec 28, 2018 •

edited by michaelcfanning

Loading

ghost Dec 28, 2018 •

edited by michaelcfanning

Loading

ghost Dec 28, 2018 •

edited by ghost

Loading

ghost Dec 28, 2018 •

edited by ghost

Loading

ghost Dec 28, 2018 •

edited by ghost

Loading

michaelcfanning Dec 28, 2018 •

edited by ghost

Loading

michaelcfanning Dec 28, 2018 •

edited by ghost

Loading

michaelcfanning Dec 28, 2018 •

edited by ghost

Loading

michaelcfanning Dec 28, 2018 •

edited by ghost

Loading

michaelcfanning commented Dec 28, 2018 •

edited by ghost

Loading