Fix #61: Generate JsonProperty attribute for properties with defaults #62

ghost · 2018-12-29T20:05:57Z

No description provided.

ghost · 2018-12-29T20:08:27Z

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

@@ -10,10 +10,13 @@ internal static class Assert
    {
        internal static void FileContentsMatchExpectedContents(
            TestFileSystem testFileSystem,
-            IDictionary<string, ExpectedContents> expectedContentsDictionary)
+            IDictionary<string, ExpectedContents> expectedContentsDictionary,
+            bool generateEqualityComparers)


generateEqualityComparers [](start = 17, length = 25)

Before this, every unit test generated an equality comparer in addition to the class, and this assertion method relied on that. The test I just added has no implications for the equality comparer, so I wanted to make it possible to write a test that just generated the class. All the other changes, except for the one new test and the actual implementation, flow from this change. #Closed

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

src/Json.Schema.ToDotNet/SyntaxNodeExtensions.cs

@@ -120,20 +120,17 @@ private static BaseTypeDeclarationSyntax AddGeneratedCodeAttribute(BaseTypeDecla
            AttributeListSyntax attributeListForGeneratedCodeAttribute =
                SyntaxFactory.AttributeList(SyntaxFactory.SingletonSeparatedList(MakeGeneratedCodeAttribute()));

-            var enumDecl = typeDecl as EnumDeclarationSyntax;
-            if (enumDecl != null)
+            if (typeDecl is EnumDeclarationSyntax enumDecl)


enumDecl [](start = 50, length = 8)

Hygiene: Use C# 7 "pattern matching". #Closed

TIL

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

Yeah, I learned that from one of the IDE diagnostics in the error window and read about it yesterday.

In reply to: 244508046 [](ancestors = 244508046,244507593)

ghost · 2018-12-29T20:11:20Z

    protected override AttributeSyntax[] GeneratePropertyAttributes(string propertyName, string serializedName, bool isRequired, object defaultValue)

This method is too long and I should refactor it into helpers like GenerateDataMemberAttribute, GenerateDefaultValueAttribute, GenerateJsonPropertyAttribute... #Pending

Refers to: src/Json.Schema.ToDotNet/ClassGenerator.cs:252 in ad3e739. [](commit_id = ad3e739, deletion_comment = False)

michaelcfanning · 2018-12-29T20:29:07Z

src/Json.Schema.ToDotNet/ClassGenerator.cs

+        private const string JsonPropertyAttributeNamespaceName = "Newtonsoft.Json";
+        private const string JsonPropertyAttributeName = "JsonProperty";
+        private const string DefaultValueHandlingPropertyName = "DefaultValueHandling";
+        private const string DefaultValueHandlingEnumerationName = "DefaultValueHandling";


ok, appears to be by design these const values are the same? #ByDesign

Yes, just me being a stickler. The property name "just happens" to be the same as the enumeration name.

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

michaelcfanning · 2018-12-29T20:34:28Z

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

@@ -1070,6 +1071,7 @@ public SNodeKind SNodeKind
        /// </summary>
        [DataMember(Name = ""intPropWithDefault"", IsRequired = false, EmitDefaultValue = false)]
        [DefaultValue(42)]
+        [JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]


DefaultValueHandling [](start = 22, length = 20)

What's your process for maintaining these tests? Is it only manually? Do you use debugger values for comparison? Do you ever write them to disk and diff? Etc.

Signed, 'Harps on a Consistent Set of Strings' #Pending

I "maintain" it by:

Doing a "Copy all" of the error message from the Tests Explorer window.

Copying it into a blank file.

Splitting the "expected" and "actual" strings into separate files.

Using Beyond Compare to convince myself that the new "actual" is correct.

Updating the test by hand.

Not fun.

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

Dear god in heaven. There are several (unfortunately diverse) techniques demonstrated in the SARIF-SDK that would alleviate this. The best is to send all output to dedicated 'expected' and 'actual' directories. Then you can diff the dir for a large suite of tests. You can copy 'actual' files into 'expected' once you confirm that you like them.

In reply to: 244508254 [](ancestors = 244508254,244508100)

michaelcfanning · 2018-12-29T20:36:23Z

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

+@"{
+  ""type"": ""object"",
+  ""properties"": {
+    ""intProp"": {


intProp [](start = 6, length = 7)

why so stingy with your identifiers? Is there anything wrong with 'integerProperty' here? readability of these string literals honestly would be improved. the double-quotes work against the terseness of your ids. fine as a TBD, of course, if you even decide to take the note #Resolved

I don't mind changing them. Doing it now...

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

ugh. sorry to put you through this. wish you had an automated process. :) i can't resist asking, how do you handle the double quote escaping? do you have any file replace magic for this?

In reply to: 244508723 [](ancestors = 244508723,244508136)

No, I just type the double-quotes. Anyway, this one is now fixed.

In reply to: 244510035 [](ancestors = 244510035,244508723,244508136)

michaelcfanning · 2018-12-29T20:37:19Z

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

+    ""stringPropWithDefault"": {
+      ""type"": ""string"",
+      ""description"": ""A string property with a default value."",
+      ""default"": ""42""


42 [](start = 21, length = 2)

you couldn't think up a better string value here? how about "thanks for all the fish" #Resolved

Oooh! I'm going to change that.

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

michaelcfanning · 2018-12-29T20:38:30Z

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

+    ""intPropWithDefault"": {
+      ""type"": ""integer"",
+      ""description"": ""An integer property with a default value."",
+      ""default"": 42


42 [](start = 19, length = 2)

what would happen, I wonder, if you set this value to something that exceeds Int32.MaxValue? Just curious about current symptoms in this circumstance #WontFix

I tried it. It silently overflows. If I set default to 5000000000 (five billion), it generates:

IntegerPropertyWithDefault = 705032704; // Seven-hundred-odd million, the excess over 2**32.

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

michaelcfanning · 2018-12-29T20:39:41Z

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

+    ""boolPropWithFalseDefault"": {
+      ""type"": ""boolean"",
+      ""description"": ""A Boolean property with a false default value."",
+      ""default"": false


false [](start = 19, length = 5)

not sure how interesting this is, since it's the .NET default. why did you include it? if this test is interesting, why not add a string property with the default of 'null'? and an integer with 0, etc? just to be comprehensive. #ByDesign

It's only there because it exercises a code branch. In Roslyn the true literal is a different SyntaxKind from the false literal, so there's a ternary operator in the code to generate one or the other. There's no similar branch for numeric values. And you can't put null for a string-valued property; null is a different data type.

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

michaelcfanning · 2018-12-29T20:41:46Z

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

+        /// A Boolean property with a false default value.
+        /// </summary>
+        [DataMember(Name = ""boolPropWithFalseDefault"", IsRequired = false, EmitDefaultValue = false)]
+        [DefaultValue(false)]


this actually an entirely unnecessary attribution, I believe, as is the JsonProperty attribute that values. For cases where .NET init saves the day, EmitDefaultValue is sufficient. whether you want to handle this case is up to you. Note that there are .NET helpers for retrieving default values of object type, I believe, which might help you create conditional logic to avoid this code emit #Pending

Nice idea. I filed #63, "Don't emit default-related attributes if the JSON schema default is the same as the .NET default", and will address it separately (but not with high priority -- it was important to get this far, for the sake of the SARIF schema changes, but we can certainly live without #63).

I found this helper:

object GetDefaultValue(Type t) { if (t.IsValueType) return Activator.CreateInstance(t); return null; }

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

michaelcfanning · 2018-12-29T20:44:40Z

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

+}";
+
+            var generator = new DataModelGenerator(_settings, _testFileSystem.FileSystem);
+            JsonSchema schema = SchemaReader.ReadSchema(Schema, TestUtil.TestFilePath);


TestUtil [](start = 64, length = 8)

'TestUtil', boo. are we programming in the 80's? you can take the engineers out of the eighties i suppose, but you can't completely take the eighties from the engineer. cf. earlier references to 'hitchhiker's guide to the galaxy'. actually published in 10/79, of course, but read by us, no doubt, in the subsequent decade #WontFix

Ugh. I'm not changing the 53 occurrences of this name. We can laugh at our younger selves, but I don't see this name as a barrier to readability. It's just a bit old-fashioned. That's why Terisa calls me "Fifties Man".

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

Born in the Fifties. Formed by the Eighties. Perhaps you enjoyed the Police song 'Born in the Fifties' at that time.

In reply to: 244512315 [](ancestors = 244512315,244508719)

michaelcfanning · 2018-12-29T20:51:00Z

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

+
+            generator.Generate(schema);
+
+            var expectedContentsDictionary = new Dictionary<string, ExpectedContents>


expectedContentsDictionary [](start = 16, length = 26)

your fancy initialization here is working against readabilty, for those not familiar with the indexing syntax you're utilizing.

Here's another way to do it:

var expectedContentsDictionary = new Dictionary<string, ExpectedContents> { { _settings.RootClassName, new ExpectedContents {ClassContents = ExpectedClass } } };

Even putting your current code on one line helps communicate what's happening better, I think.

var expectedContentsDictionary = new Dictionary<string, ExpectedContents> { [_settings.RootClassName] = new ExpectedContents { ClassContents = ExpectedClass } };

summary of note: improve readability if you can
#ByDesign

Putting it on one line is ok if there's only one initializer, but it spreads out too far when there is more than one:

var expectedContentsDictionary = new Dictionary<string, ExpectedContents> { [_settings.RootClassName] = new ExpectedContents { ClassContents = ExpectedRootClass, ComparerClassContents = ExpectedRootComparerClass }, ["Color"] = new ExpectedContents { ClassContents = ExpectedColorClass, ComparerClassContents = ExpectedColorComparerClass } };

IMO this is fine, and I'm not sympathetic to "hard to read if you don't know the language" ;-)

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

Code is written to serve the reader and not the author. Patterns at the leading edge of the language are often confusing to people. There often isn't common convention around how best to expresss them, as various codesmiths are tinkering. My note here is only that this fairly abstruse initialization syntax is not particularly readable.

In reply to: 244511949 [](ancestors = 244511949,244509144)

michaelcfanning · 2018-12-29T20:53:00Z

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

@@ -3464,5 +3615,10 @@ public sealed class C
            string actual = generator.Generate(schema);
            actual.Should().Be(Expected);
        }
+
+        private void VerifyGeneratedFileContents(IDictionary<string, ExpectedContents> expectedContentsDictionary)


VerifyGeneratedFileContents [](start = 21, length = 27)

why did you do this? I am suspicious of helpers that consist of a single line of code. do you project more shared validation here in future?

the downside of what you've done, taken to an extreme, is endless stepping into/out during debugging sessions. as you know, i approach unit-testing to maximize debuggability/ease of update. #ByDesign

Once I added the third argument, and two of the three arguments were instance fields, I felt this was more compact.

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

michaelcfanning · 2018-12-29T21:02:36Z

src/Json.Schema.ToDotNet.UnitTests/Hints/DictionaryHintTests.cs

@@ -1245,7 +1245,7 @@ public partial class Def2
                }
            };

-            Assert.FileContentsMatchExpectedContents(TestFileSystem, expectedContentsDictionary);
+            Assert.FileContentsMatchExpectedContents(TestFileSystem, expectedContentsDictionary, Settings.GenerateEqualityComparers);


Settings [](start = 97, length = 8)

'Settings' is too generic a term here. #WontFix

michaelcfanning

michaelcfanning · 2018-12-29T21:20:19Z

    protected override AttributeSyntax[] GeneratePropertyAttributes(string propertyName, string serializedName, bool isRequired, object defaultValue)

agreed, and fine to defer. I've approved this change which in the main is just fine. I gave you the full bore review, of course, since it's the weekend and I have the luxury of focusing on it. :) I will continue to track your changes/any discussion but fire away when you are satisfied.

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

Refers to: src/Json.Schema.ToDotNet/ClassGenerator.cs:252 in ad3e739. [](commit_id = ad3e739, deletion_comment = False)

ghost · 2018-12-29T21:26:17Z

    protected override AttributeSyntax[] GeneratePropertyAttributes(string propertyName, string serializedName, bool isRequired, object defaultValue)

Thanks for the thorough look. I'll take a look at yours after lunch.

In reply to: 450520861 [](ancestors = 450520861,450516997)

Refers to: src/Json.Schema.ToDotNet/ClassGenerator.cs:252 in ad3e739. [](commit_id = ad3e739, deletion_comment = False)

ghost · 2018-12-29T21:50:57Z

@michaelcfanning Now that this is merged, can you update the version number and publish the new package? #Pending

michaelcfanning · 2018-12-29T22:50:25Z

Yes.

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

* 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 3 commits December 28, 2018 16:56

Add dummy test; make equality comparer generation optional.

f3ce4a2

Create and skip failing test.

e008866

Implement #61.

ad3e739

ghost requested a review from michaelcfanning December 29, 2018 20:05

ghost commented Dec 29, 2018

View reviewed changes

Fix broken test and extend new test.

e8163c5

michaelcfanning reviewed Dec 29, 2018

View reviewed changes

Address PR feedback.

4ec4083

michaelcfanning approved these changes Dec 29, 2018

View reviewed changes

ghost merged commit fb49dd8 into develop Dec 29, 2018

ghost deleted the users/lgolding/issue-61-jproperty-attribute branch December 29, 2018 21:49

This pull request was closed.


		generator.Generate(schema);

		var expectedContentsDictionary = new Dictionary<string, ExpectedContents>

Fix #61: Generate JsonProperty attribute for properties with defaults #62

Fix #61: Generate JsonProperty attribute for properties with defaults #62

Conversation

ghost commented Dec 29, 2018

ghost Dec 29, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Dec 29, 2018 • edited by michaelcfanning 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 29, 2018 • edited by ghost Loading

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 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 29, 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

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 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 29, 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 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 29, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning commented Dec 29, 2018

ghost commented Dec 29, 2018

ghost commented Dec 29, 2018 • edited by michaelcfanning Loading

michaelcfanning commented Dec 29, 2018

ghost Dec 29, 2018 •

edited by michaelcfanning

Loading

ghost Dec 29, 2018 •

edited by michaelcfanning

Loading

ghost commented Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

michaelcfanning Dec 29, 2018 •

edited by ghost

Loading

ghost commented Dec 29, 2018 •

edited by michaelcfanning

Loading