* add .NET 5.0 support by trivalik · Pull Request #286

* add .NET 5.0 support by trivalik · Pull Request #286 · Shane32/QRCoder

Add support for .NET 5, except for the UWP project.

Choose a reason for hiding this comment

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

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?

		if (!string.IsNullOrEmpty(mandateId))
		bezahlCodePayload += $"mandateid={ Uri.EscapeDataString(mandateId)}&";
		if (dateOfSignature != null)
		if (dateOfSignature != DateTime.MinValue)

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

		bezahlCodePayload += $"periodictimeunit={periodicTimeunit}&";
		bezahlCodePayload += $"periodictimeunitrotation={periodicTimeunitRotation}&";
		if (periodicFirstExecutionDate != null)
		if (periodicFirstExecutionDate != DateTime.MinValue)

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

		if (periodicFirstExecutionDate != DateTime.MinValue)
		bezahlCodePayload += $"periodicfirstexecutiondate={periodicFirstExecutionDate.ToString("ddMMyyyy")}&";
		if (periodicLastExecutionDate != null)
		if (periodicLastExecutionDate != DateTime.MinValue)

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

After review I found that QRCoder.csproj specifies the version in new style, but disable it use by <GenerateAssemblyInfo>false</GenerateAssemblyInfo>.

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 DateTime? nothing to the local member with type DateTime. This lead to that only the default value is assigned, this is DateTime.MinValue or if you want default(DateTime).

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.

@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?