SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
877 stars 45 forks source link

Add support for `JsonSerializerOptions.NumberHandling` #494

Closed viceroypenguin closed 1 year ago

viceroypenguin commented 1 year ago

This PR adds support for JsonSerializerOptions.NumberHandling on net5+, while retaining old behavior on other frameworks.

When supported (net5+) and Conversions.TreatNumberAsStringInSystemTextJson is not set, then the converter will read JsonSerializerOptions.NumberHandling to determine whether to allow parsing numeric values from string tokens, similar to the effect JsonSerializerOptions.NumberHandling has on deserializing numeric primitives.

@SteveDunn I was not able to get the verify files updated correctly. I ran the test.ps1, and it failed due to TZ issues, and would not generate any of the verify files.

I have verified that this change works correctly for my situation; if this change is not desired, feel free to close.

Fixes #493

viceroypenguin commented 1 year ago

@SteveDunn Would you be opposed to the migration of the models to Scriban or something? This might make some of these changes easier if we can pass in a tfm from the Compilation and affect which part of the model is used (only using the property name converters on tfm6+, etc.). Obviously I'd offer that in a separate PR if you were amenable.

SteveDunn commented 1 year ago

@SteveDunn Would you be opposed to the migration of the models to Scriban or something? This might make some of these changes easier if we can pass in a tfm from the Compilation and affect which part of the model is used (only using the property name converters on tfm6+, etc.). Obviously I'd offer that in a separate PR if you were amenable.

Thanks for the PR! Yes, it is painful when making a change to the generated code. There is a flag that can be set in .build.ps1. I've documented it in the readme, but I'll also add it to the wiki. It is: .\Build.ps1 -v "Minimal" -resetSnapshots $true

This deletes all of the snapshots and treats all of the generated code as the correct code from now on.

I've never heard of Scriban, but I'll take a look. Thanks for the recommendation.

SteveDunn commented 1 year ago

Also, could you tell me what tests are failing due to time-zone differences? I thought (hoped!) that I'd fixed all of those.

viceroypenguin commented 1 year ago

Also, could you tell me what tests are failing due to time-zone differences? I thought (hoped!) that I'd fixed all of those.

ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets appears to be the offending test. I get four failures for this test (note, I am in CST, so -06:00 in the winter):

############################################
****  Running end to end tests with the local version of the NuGet package:999.9.30960716
############################################

Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net461\ConsumerTests.dll (.NETFramework,Version=v4.6.1)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:14.79]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [492 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:   995, Skipped:     0, Total:   996, Duration: 10 s - ConsumerTests.dll (net461)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\netcoreapp3.1\ConsumerTests.dll (.NETCoreApp,Version=v3.1)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:03.02]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [181 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:   995, Skipped:     0, Total:   996, Duration: 882 ms - ConsumerTests.dll (netcoreapp3.1)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net5.0\ConsumerTests.dll (.NETCoreApp,Version=v5.0)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:03.09]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [220 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:   995, Skipped:     0, Total:   996, Duration: 1 s - ConsumerTests.dll (net5.0)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net6.0\ConsumerTests.dll (.NETCoreApp,Version=v6.0)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:08.00]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [147 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:  1023, Skipped:     0, Total:  1024, Duration: 2 s - ConsumerTests.dll (net6.0)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net7.0\ConsumerTests.dll (.NETCoreApp,Version=v7.0)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:02.11]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [157 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at FluentAssertions.Execution.AssertionScope.Dispose()
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

Failed!  - Failed:     1, Passed:  1073, Skipped:     0, Total:  1074, Duration: 1 s - ConsumerTests.dll (net7.0)
Exec:
At C:\Users\<username>\source\repos\viceroypenguin\Vogen\Build.ps1:50 char:9
+         throw ("Exec: " + $errorMessage)
+         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (Exec: :String) [], RuntimeException
    + FullyQualifiedErrorId : Exec:
viceroypenguin commented 1 year ago

@SteveDunn any chance this PR can be merged and a new release dropped soon?

SteveDunn commented 1 year ago

Hi @viceroypenguin - sorry for slow responses lately, it's been busy in my day job and I'm trying to finish up #479 which has taken much longer than expected!

I'll hopefully be finished within a day or so, I'll release both at the weekend, if that's OK with you?

SteveDunn commented 1 year ago

Merged just now - thank you again @viceroypenguin ! I'll fix those tests soon too - I think I know what's wrong.

SteveDunn commented 1 year ago

Should be in nuget soon v. 3.0.22

viceroypenguin commented 1 year ago

Thank you @SteveDunn!