Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DateOnly / TimeOnly parameters is TestCaseUsage #821

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Dreamescaper
Copy link
Member

@Dreamescaper Dreamescaper commented Dec 31, 2024

Fixes #820 .

I had to update test project to use net8.0 here (to be able to actually test DateOnly / TimeOnly parameters), hope it's ok.

new()
{
(typeof(bool), new Lazy<TypeConverter>(() => new BooleanConverter())),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated mapping per TypeConverter updates:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs#L154

Had to update it to string from Type, as netstandard2.0 does not have DateTime/DateOnly/etc types.

@@ -12,7 +12,6 @@
<PackageReference Include="Nullable" Version="1.3.1" PrivateAssets="all" />
<PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="3.11.0" />
<PackageReference Include="System.ComponentModel.TypeConverter" Version="4.3.0" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package is not needed on netstandard2.0

@Dreamescaper
Copy link
Member Author

Appveyor build failed, but I'm not really fure why:
The running command stopped because the preference variable "ErrorActionPreference" or common parameter is set to Stop: .NETCoreApp,Version=v8.0

@mikkelbu
Copy link
Member

Appveyor build failed, but I'm not really fure why: The running command stopped because the preference variable "ErrorActionPreference" or common parameter is set to Stop: .NETCoreApp,Version=v8.0

My time is very limited today due to new year's eve, but perhaps this is related to the cake tool? At least the bump to version 5.0 fails with
https://ci.appveyor.com/project/mikkelbu/nunit-analyzers/builds/51103770

.\build.ps1 --target="Appveyor" --configuration="Release" --targetFramework="netstandard2.0"
C:\Users\appveyor\AppData\Local\Temp\1\243f61e3-174d-4243-bbfd-5f4dd437f312\restore.csproj : error NU1202: Package Cake.Tool 5.0.0 is not compatible with net6.0 (.NETCoreApp,Version=v6.0) / any. Package Cake.Tool 5.0.0 supports:
C:\Users\appveyor\AppData\Local\Temp\1\[24](https://ci.appveyor.com/project/mikkelbu/nunit-analyzers/builds/51103770#L24)3f61e3-174d-4243-bbfd-5f4dd437f312\restore.csproj : error NU1202:   - net8.0 (.NETCoreApp,Version=v8.0) / any
C:\Users\appveyor\AppData\Local\Temp\1\243f61e3-174d-4243-bbfd-5f4dd437f312\restore.csproj : error NU1202:   - net9.0 (.NETCoreApp,Version=v9.0) / any
The running command stopped because the preference variable "ErrorActionPreference" or common parameter is set to Stop: Package "cake.tool" failed to restore, due to Microsoft.DotNet.ToolPackage.ToolPackageException: The tool package could not be restored.

So perhaps bumping the cake tool will solve the problem (I've not checked if there are any breaking changes between 3.2.0 and 5.0.0. Perhaps bumping to version 4 will be enough?

@mikkelbu mikkelbu changed the title Fix DateOnly / TimeOnly parameters is TestCaseUsage WIP: Fix DateOnly / TimeOnly parameters is TestCaseUsage Jan 1, 2025
@mikkelbu mikkelbu changed the title WIP: Fix DateOnly / TimeOnly parameters is TestCaseUsage Fix DateOnly / TimeOnly parameters is TestCaseUsage Jan 1, 2025
@mikkelbu mikkelbu marked this pull request as draft January 1, 2025 16:27
@mikkelbu mikkelbu marked this pull request as ready for review January 1, 2025 16:27
@mikkelbu
Copy link
Member

mikkelbu commented Jan 1, 2025

@Dreamescaper Sorry about the WIP and moving the PR to draft and back again. I was hoping that this was enough to retrigger the builds. I've updated cake to version 4 on master and I hope this is enough to solve the problem with AppVeyor

@Dreamescaper
Copy link
Member Author

@mikkelbu
It seems to work just fine after merge from master, thanks for the help!

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look sound @Dreamescaper. I'm a bit unsure why we need the bump to .NET8 as DateOnly and TimeOnly were already part of .NET 6, but I think I might be missing something obvious :).

Not that I've against the bump - I just thought from #677 that it would require update of the Roslyn Analyzer as well as larger consequential changes.

I've approved it, but I would prefer if @manfred-brands can take a second look before we merge this as I think he has a better understanding of the bump than I have (also I would like to test the generated nuget package locally).

@Dreamescaper
Copy link
Member Author

Ooops. My bad. I don't know why I decided that I need to update to NET8.
I suggest to extract the bump to a separate PR.

@Dreamescaper Dreamescaper force-pushed the fix_dateonly_parameters branch from 8db616c to 8cc9968 Compare January 6, 2025 13:52
@Dreamescaper Dreamescaper mentioned this pull request Jan 6, 2025
@Dreamescaper Dreamescaper force-pushed the fix_dateonly_parameters branch from 8cc9968 to fc1f568 Compare January 6, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NUnit1001 compains about DateOnly parameters
2 participants