FEATURE: Add new option `--generate-integer-as` #158

shaopeng-gh · 2022-08-22T21:35:56Z

FEATURE: Add new option --generateintegeras = int | long | biginteger | auto and
default as int to minimize impact to existing users.
Existing tests are passing and added test for the new option.
For SARIF SDK, we will be using auto to generate int/biginteger as appropriate.

shaopeng-gh · 2022-08-22T23:45:20Z

src/Json.Schema/JsonSchema.cs

@@ -108,7 +108,9 @@ public JsonSchema(JsonSchema other)
            MinLength = other.MinLength;
            Format = other.Format;
            MultipleOf = other.MultipleOf;
+            Minimum = other.Minimum;


Minimum = other.Minimum;

This is an existing bug, it is not set.
So I can not use Max + Min for the auto #Resolved

Ouch. So we clearly do not have sufficient tests. For this kind of testing, I typically use advanced reflection techniques to instantiate object instances. I'd populate them with random values. Then I'd test the ctor here and the basic equatable functionality. Something to keep in mind if we have time.

Created a follow-up issue in the other comment.

shaopeng-gh · 2022-08-22T23:54:27Z

src/Json.Schema.ToDotNet.UnitTests/TestData/Integer.schema.json

+      "type": "integer",
+      "maximum": 0
+    },
+    "integerProperty_min_0_max_int64times10": {


int64times10

for int32 it is add1, but int64 it is times10,
the reason is min and max is double type,
double is less accurate when number is big.

min and max can not be changed to BigInterger because
min and max definition is not integer only.
#Closed

marmegh · 2022-08-23T00:10:55Z

src/Json.Schema.ToDotNet/DataModelGeneratorSettings.cs

+        /// Gets or sets a value indicating what C# type to generate for Json type
+        /// <code>Integer</code>.
+        /// </summary>
+        public string GenerateIntegerAs { get; set; }


string

Should this be a string or should it be an enum? #Closed

good point, changed to enum.

marmegh · 2022-08-24T16:45:25Z

src/ReleaseHistory.md

@@ -1,5 +1,8 @@
 # Microsoft Json Schema Packages

+## **Unreleased**
+* FEATURE: Add new option `--generateintegeras = int | long | biginteger | auto` and default as `int`. [#158](https://github.com/microsoft/jschema/pull/158)


generateintegeras

Please include the dashes as they appear in Options.cs, this should look something like --generate-integer-as to avoid confusion and help with readability. #Closed

yes! fixed.

marmegh · 2022-08-24T16:49:23Z

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

+            GeneratesPropertiesWithIntegerTypes_Helper(GenerateIntegerOption.Auto, ExpectedClass_Auto);
+            GeneratesPropertiesWithIntegerTypes_Helper(GenerateIntegerOption.BigInteger, ExpectedClass_BigInteger);
+            GeneratesPropertiesWithIntegerTypes_Helper(GenerateIntegerOption.Long, ExpectedClass_Long);
+            GeneratesPropertiesWithIntegerTypes_Helper(GenerateIntegerOption.Int, ExpectedClass_Int);


These all seem like happy path test cases. Can we explicitly have a test case where the user has set --generate-integer-as int but the actual value will cause an int overflow? #Closed

Additional test case needed: --generate-integer-as is not set and default applies. Both happy path and with an int overflow.

the input is a .json schema, and output is C# classes files, there is no actual value involved, the goal is user specify to generate int in his C# class and we output int.

added the test that when user does not include the parameter, we will be getting int.

marmegh · 2022-08-24T16:49:40Z

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

+            GeneratesPropertiesWithIntegerTypes_Helper(GenerateIntegerOption.Int, ExpectedClass_Int);
+        }
+
+


Nit: remote extra blank line. #Closed

marmegh

marmegh · 2022-08-24T23:34:52Z

Please have @cfaucon review before merging.

In reply to: 1226600870

cfaucon · 2022-08-25T20:35:40Z

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

+            {
+                ("--schema-name Sarif --schema-file-path \"C:\\sarif.json\" --output-directory \"C:\\Autogenerated\" --namespace-name Microsoft.CodeAnalysis.Sarif --root-class-name SarifLog", (GenerateIntegerOption?)GenerateIntegerOption.Int, null),
+                ("--schema-name Sarif --schema-file-path \"C:\\sarif.json\" --output-directory \"C:\\Autogenerated\" --namespace-name Microsoft.CodeAnalysis.Sarif --root-class-name SarifLog --generate-integer-as=Int", (GenerateIntegerOption?)GenerateIntegerOption.Int, null),
+                ("--schema-name Sarif --schema-file-path \"C:\\sarif.json\" --output-directory \"C:\\Autogenerated\" --namespace-name Microsoft.CodeAnalysis.Sarif --root-class-name SarifLog --generate-integer-as=int", (GenerateIntegerOption?)GenerateIntegerOption.Int, null),


What are there case variations for int type and not the others? Is there a more dynamic way you can generate these test inputs since the majority of the string is the same and at the same time have it generate all the case variants if you are looking to support case insensitivity #Closed

Did some reflector and removed the duplication and simplified the input.
The reason the case sensitive test does not cover all is I was just to simply verify the case insensitive flag is set, case insensitive is build-in option for the ConmandLine package. Let me know I can generate all possible case combination of all the cases if we like.

cfaucon · 2022-08-25T20:40:20Z

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

+                    .MapResult(
+                    options =>
+                    {
+                        testCase.expectedGenerateIntegerAs.Should().NotBeNull();


The first test case to failure its conditions fails the test as a whole, and the developer will not know if any other testcases in the test are broken too. What does the test failure output look like? Is it clear what test case failed?

Recommend making it do its verification checks and append to a failure string builder, and at the end of the test emits all test case failures in a clear manner that is easy to debug and ensure we tested everything. #Closed

Yes! Fixed.

cfaucon · 2022-08-25T20:41:31Z

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

+
+        private void GeneratesPropertiesWithIntegerTypes_Helper(GenerateIntegerOption generateIntegerAs, string expectedClass)
+        {
+            // Arrange.


Are these code comments necessary? #Closed

cfaucon · 2022-08-25T20:45:11Z

src/Json.Schema.ToDotNet.Cli/Options.cs

@@ -92,6 +92,13 @@ internal class Options
            Required = false)]
        public bool GenerateCloningCode { get; set; }

+        [Option(
+            "generate-integer-as",


Very meta question here. In terms of the option, generate-integer implies its an integer which in a way conflict with the fact it might be say a long. Is the proper term to use here number? as in --generate-number-as which could be set to integer, long, etc. Open to discussion this is very meta #Closed

good question, add my inputs to the discussion:
I originally plan to use --generate-json-integer-type-as-csharp-type = long
This includes the full info but I think looks long so I tried to shorten it. logically I will also use --generate-number-as but it has a problem here because json schema has a type number so to avoid confusion we should avoid the word number,
Currently I think like generate-json-integer-as or some longer version that is more clear. @cfaucon let me know your take,

Made the proposed change.

michaelcfanning · 2022-09-02T16:23:05Z

src/ReleaseHistory.md

@@ -2,6 +2,7 @@

 ## **Unreleased**
 * Loosen Newtonsoft.JSON minimum version requirement from v13.0.1 to v9.0.1 [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json/9.0.1). [#157](https://github.com/microsoft/jschema/pull/157)
+* FEATURE: Add new option `--generate-json-integer-as = int | long | biginteger | auto` and default as `int`. [#158](https://github.com/microsoft/jschema/pull/158)


Nice command-line argument name, by the way, very descriptive.

michaelcfanning · 2022-09-02T16:30:29Z

    public bool Equals(JsonSchema other)

Hey, is there a bug in this method due to failure to account for Minimum or ExclusiveMinimum??

If so, let's please do add some advanced tests (not hard-coded by member name) to find/fix this bug.

The reflection-based tests should trigger anytime we add a new property/member to this class without properly updating the object copying ctor/Equals methods.

You can take this in a follow on change...

In reply to: 1235698310

Refers to: src/Json.Schema/JsonSchema.cs:621 in ea9eec9. [](commit_id = ea9eec9, deletion_comment = False)

michaelcfanning · 2022-09-02T16:31:33Z

src/Json.Schema.ToDotNet/DataModelGeneratorSettings.cs

@@ -82,6 +82,12 @@ public class DataModelGeneratorSettings
        /// </summary>
        public bool GenerateCloningCode { get; set; }

+        /// <summary>
+        /// Gets or sets a value indicating what C# type to generate for Json type


Gets or sets a value indicating what C# type to generate for Json type

Use this:

Gets or sets a value indicating what C# type to emit for Json integers. #Resolved

michaelcfanning · 2022-09-02T16:40:29Z

src/Json.Schema.ToDotNet/DataModelGeneratorSettings.cs

+        /// Gets or sets a value indicating what C# type to generate for Json type
+        /// <code>Integer</code>.
+        /// </summary>
+        public GenerateJsonIntegerOption GenerateJsonIntegerAs { get; set; }


GenerateJsonIntegerOption

So where's our handling for the JSON 'number' type?

We need to first review our SARIF schema to make sure we don't need a number type. Look at this one 'rank'. The specification says it's a value from 0.0 to 100.0 and provides an explicit example of 0.7 which is conclusively not an integer (in JSON, a 0 decimal value, e.g., 100.0 can be interpreted as an integer).

I think it's clear we'll need generate-json-number-as. :)

https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317663 #Resolved

Created an issue to track: Evaluate and add option --generate-json-number-as

michaelcfanning · 2022-09-02T18:32:14Z

src/Json.Schema.ToDotNet/PropertyInfoDictionary.cs

@@ -654,5 +668,50 @@ private TypeSyntax GetConcreteType(string propertyName, string interfaceName)

            return type;
        }
+
+        internal TypeSyntax MakeProperIntegerType(GenerateJsonIntegerOption generateJsonIntegerAs,


MakeProperIntegerType

I'm not entirely sure about this approach. Let me think about it. :) Let's get this change in, then work on guid & number and I will discuss this further with you.

Ok. Looking at UUID PR now. Thanks.

michaelcfanning

shaopeng-gh · 2022-09-02T20:28:00Z

    public bool Equals(JsonSchema other)

Created a issue to track: Examine Equals methods and add tests

In reply to: 1235698310

Refers to: src/Json.Schema/JsonSchema.cs:621 in ea9eec9. [](commit_id = ea9eec9, deletion_comment = False)

shaopeng-gh added 3 commits August 21, 2022 23:44

GenerateIntegerAs

b9c6a58

Add tests

cb9c214

Add Release History

8707b7f

shaopeng-gh requested review from michaelcfanning, marmegh, cfaucon, EasyRhinoMSFT and yongyan-gh August 22, 2022 21:35

shaopeng-gh commented Aug 22, 2022

View reviewed changes

marmegh reviewed Aug 23, 2022

View reviewed changes

change GenerateIntegerOption to Enum

4a46021

marmegh reviewed Aug 24, 2022

View reviewed changes

shaopeng-gh added 2 commits August 24, 2022 15:50

add CommandLine test

14c0f0d

use null type for test

25f33da

marmegh approved these changes Aug 24, 2022

View reviewed changes

shaopeng-gh changed the title ~~FEATURE: Add new option --generateintegeras~~ FEATURE: Add new option --generate-integer-as Aug 24, 2022

cfaucon reviewed Aug 25, 2022

View reviewed changes

shaopeng-gh added 5 commits August 28, 2022 17:45

Changed to --generate-json-integer-as.

3fbb6b8

merge from main

ae3618f

use GenerateJsonIntegerOption

2b1f706

simplify argsString

d04d6ec

Remove not needed cast

ea9eec9

michaelcfanning reviewed Sep 2, 2022

View reviewed changes

michaelcfanning approved these changes Sep 2, 2022

View reviewed changes

This was referenced Sep 2, 2022

Evaluate and add option --generate-json-number-as #160

Closed

Examine Equals methods and add tests #161

Open

update release history text and member desc text accoding to feedback

9f6c801

shaopeng-gh requested a review from michaelcfanning September 2, 2022 20:42

michaelcfanning merged commit d6d98b6 into main Oct 4, 2022

michaelcfanning deleted the users/shaopeng-gh/bigint2 branch October 6, 2022 23:12

shaopeng-gh mentioned this pull request Oct 7, 2022

Releasing 2.0.0 #163

Merged

shaopeng-gh mentioned this pull request Oct 27, 2022

See if jschema can change to support Big Integer instead of max 64 bit integer #144

Closed

		GeneratesPropertiesWithIntegerTypes_Helper(GenerateIntegerOption.Int, ExpectedClass_Int);
		}

FEATURE: Add new option --generate-integer-as #158

FEATURE: Add new option --generate-integer-as #158

Conversation

shaopeng-gh commented Aug 22, 2022

shaopeng-gh Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

marmegh Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh left a comment

Choose a reason for hiding this comment

marmegh commented Aug 24, 2022 • edited by shaopeng-gh Loading

cfaucon Aug 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfaucon Aug 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfaucon Aug 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfaucon Aug 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Sep 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Sep 2, 2022 • edited by shaopeng-gh Loading

michaelcfanning Sep 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Sep 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

shaopeng-gh commented Sep 2, 2022 • edited Loading

FEATURE: Add new option `--generate-integer-as` #158

FEATURE: Add new option `--generate-integer-as` #158

shaopeng-gh Aug 22, 2022 •

edited

Loading

shaopeng-gh Aug 22, 2022 •

edited

Loading

marmegh Aug 23, 2022 •

edited

Loading

marmegh Aug 24, 2022 •

edited

Loading

marmegh Aug 24, 2022 •

edited

Loading

marmegh Aug 24, 2022 •

edited

Loading

marmegh commented Aug 24, 2022 •

edited by shaopeng-gh

Loading

cfaucon Aug 25, 2022 •

edited by shaopeng-gh

Loading

cfaucon Aug 25, 2022 •

edited by shaopeng-gh

Loading

cfaucon Aug 25, 2022 •

edited by shaopeng-gh

Loading

cfaucon Aug 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Sep 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning commented Sep 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Sep 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Sep 2, 2022 •

edited by shaopeng-gh

Loading

shaopeng-gh Sep 2, 2022 •

edited

Loading

shaopeng-gh commented Sep 2, 2022 •

edited

Loading