manuelbl / SwissQRBill.NET

.NET library for Swiss QR bill payment slips (aka QR-Rechnung)
MIT License
86 stars 32 forks source link

Pull IToByteArray up to ICanvas #22

Closed 0xced closed 2 years ago

0xced commented 4 years ago

So that we can write canvas.ToByteArray() instead of (canvas as IToByteArray)?.ToByteArray() thus ensuring that the returned byte[] is never null.

This may seem like an odd pull request so let me explain. I started playing with enabling Nullable reference types on this project. This generates a lot of warnings so I decided against a huge pull request and instead, break them into smaller pull requests for easier reviewing.

manuelbl commented 4 years ago

While your proposed change would make the code somewhat simpler and cleaner, it's a breaking change. I'm unsure if it worth the improvement.

You mention the use of nullable reference types as a goal. This feature of C# 8.0 requires .NET Core 3 as the minimum. It's not supported on .NET Framework of any version. So this goal isn't achievable at the moment.

So at the moment, I'm not inclined to accept the pull request.

0xced commented 4 years ago

I was not sure if this was a breaking change or not. Anyway, a direct cast (IToByteArray)canvas would get rid of the warning and would not be a breaking change!

This feature of C# 8.0 requires .NET Core 3 as the minimum.

Actually, nullable reference types work on .NET Framework. I have them enabled in my current project targeting .NET Framework and using some .NET Standard 2.0 libraries. And if you need the new attributes, such as NotNullIfNotNullAttribute, you can use the Nullable NuGet package.

I think it would be really nice to enable them as they really help to catch bugs. I got caught by ValidationMessage.MessageParameters being null instead of empty a few minutes after I tried SwissQRBill.NET (I know it's documented, but still…).

Would accept pull request(s) knowing now that nullable reference types work on NET Standard >= 1.0 and .NET Framework >= 2.0?

manuelbl commented 4 years ago

Will this cause any interoperability issues such as:

0xced commented 4 years ago
  • The library can only be used with projects compiled with C# 8.0.

No. You can compile a .NET Standard 2.0 library with nullable reference types and C# 8 and this library can be used on a project using any version of the C# language. Of course you won't get the benefits of compiler warnings when misusing nullable reference types if you don't enable C# 8.

  • The library (compiled with the NuGet package for .NET Standard 2.0) cannot be used with .NET Core 3 as the attributes conflict.

No. The library is perfectly usable in both a .NET Framework 4.7.2+ app and a .NET Core 3+ app.

  • The library can be used with .NET Core 3 but nullability diagnostics do not work as .NET Core 3 expects another set of attributes.

No. Diagnostics work as expected. You can even get diagnostics to work on a .NET Framework project by enabling nullable reference types and using C# 8 like this:

<PropertyGroup>
  <Nullable>enable</Nullable>
  <LangVersion>8.0</LangVersion>
</PropertyGroup>
manuelbl commented 4 years ago

I've created a branch nullability-annotations and merged your PR into it. On the branch, C# 8.0 and nullable reference types are already enabled.

We can work on this branch until the nullability work is complete.

manuelbl commented 2 years ago

Merged to branch "v3".

0xced commented 2 years ago

Closing since it was merged in commit 6602eef65b16c58d651cc1436a17508ec5381bb5.