* add .NET 5.0 support #286
Conversation
There was a problem hiding this comment.
Thanks for your effort and all the changes. In general I'm already open for changes that make QRCoder future-proof. Nevertheless, due to the huge userbase I want to ensure that every change is not (or minimal) harmful for existing users. I don't want to early adopt at any price. Thus, before merging your changes, I like to clarify some points on your changes. Can you check my review comments?
| @@ -1552,7 +1557,7 @@ public override string ToString() | |||
| bezahlCodePayload += $"creditorid={ Uri.EscapeDataString(creditorId)}&"; | |||
| if (!string.IsNullOrEmpty(mandateId)) | |||
| bezahlCodePayload += $"mandateid={ Uri.EscapeDataString(mandateId)}&"; | |||
| if (dateOfSignature != null) | |||
| if (dateOfSignature != DateTime.MinValue) | |||
There was a problem hiding this comment.
The contructor takes nullable DateTimes ([...] , DateTime? dateOfSignature = null, [...] so I guess the dates should be checked against null or does ... != Datetime.MinValue also check for null?
There was a problem hiding this comment.
DateTime is a struct. It cannot be null. Also a warning appeared for this after add of .NET 5. I didn't check datatype of dateOfSignature. Nullable is not null, it is a class arround. So we have to check it by: dateOfSignature.HasValue
I will take care.
There was a problem hiding this comment.
Thanks for the feedback. The documentation states that for nullable types both, checking for null and checking for "HasValue" works: https://docs.microsoft.com/de-de/dotnet/csharp/language-reference/builtin-types/nullable-value-types
(Check second and third example.)
So I suggest we change the code to:
| if (dateOfSignature != DateTime.MinValue) | |
| if (dateOfSignature.HasValue && dateOfSignature >= DateTime.MinValue) |
There was a problem hiding this comment.
I guess we have another issue here. The constructor does already convert the Nullable type to a regulare DateTime. In this case, nothing is assign and it will be the default value, which is probably DateTime.MinValue. This means there is no relation between your comment and this compare.
There was a problem hiding this comment.
The constructor does already convert the Nullable type to a regulare DateTime.
But it does so, only if dateOfSignature != null otherwise the conversion isn't run.
| if (authority == AuthorityType.periodicsinglepayment || authority == AuthorityType.periodicsinglepaymentsepa) | ||
| { | ||
| bezahlCodePayload += $"periodictimeunit={periodicTimeunit}&"; | ||
| bezahlCodePayload += $"periodictimeunitrotation={periodicTimeunitRotation}&"; | ||
| if (periodicFirstExecutionDate != null) | ||
| if (periodicFirstExecutionDate != DateTime.MinValue) |
There was a problem hiding this comment.
The contructor takes nullable DateTimes ([...] , DateTime? periodicFirstExecutionDate = null, [...] so I guess the dates should be checked against null or does ... != Datetime.MinValue also check for null?
There was a problem hiding this comment.
DateTime is a struct. It cannot be null. Also a warning appeared for this after add of .NET 5. I didn't check datatype of dateOfSignature. Nullable is not null, it is a class arround. So we have to check it by: dateOfSignature.HasValue
I will take care.
| bezahlCodePayload += $"periodicfirstexecutiondate={periodicFirstExecutionDate.ToString("ddMMyyyy")}&"; | ||
| if (periodicLastExecutionDate != null) | ||
| if (periodicLastExecutionDate != DateTime.MinValue) |
There was a problem hiding this comment.
The contructor takes nullable DateTimes ([...] , DateTime? periodicLastExecutionDate = null, [...] so I guess the dates should be checked against null or does ... != Datetime.MinValue also check for null?
There was a problem hiding this comment.
DateTime is a struct. It cannot be null. Also a warning appeared for this after add of .NET 5. I didn't check datatype of dateOfSignature. Nullable is not null, it is a class arround. So we have to check it by: dateOfSignature.HasValue
I will take care.
|
After review I found that QRCoder.csproj specifies the version in new style, but disable it use by |
|
My final change is now up. |
|
Hi @trivalik I reviewed the changes again. The only thing unsolved, we should handle before merging, is the DateTime/DateTime? part. Either your changes (... != MinValue) should be rolled back or we should add some kind of null/HasValue check, |
|
As I wrote above, your constructor does assign for the null value of |
|
Hi @trivalik , the merge went well. I just had to fix one, two things in the QRCoderTests project. But unfortunately another problem arised. For builds and Nuget publishing I use the services of MyGet (https://www.myget.org/). (Everytime a commit runs into this Github repo, a build is done at MyGet. Thus we have a package stream for all the "nightly" builds.) Unfortunately Myget doesn't support the target "net5.0-windows" by now. So I thought about removing "net5.0-windows" as target for the moment (while keeping net5.0). Am I correct that the only thing (which isn't handled by net5.0) we would loose is the support of XamlQRCode for net5.0 users? |
|
Yes. WPF XAML is Windows only. Maybe some nuget packages could cover it, but here I am not the expert. |
|
Thanks for your feedback. I'll check in parallel if moving the automatic builds from MyGet to Github Actions/Runners is possible. If I can figure out how this works (especially for my multi target builds), we can keep the net5.0-windows builds :-) |
|
Is the 1.4.2 version including the support for .NET 5.0 going to be published as a NuGet package in the short term? |
|
@martingra 1.4.2 will include .NET 5.0 support. Release on Nuget is planned during the next 14 days. |
Hi! Is there a new date planned to deploy 1.4.2 on Nuget? |
Add support for .NET 5, except for the UWP project.