JoshClose / CsvHelper

Library to help reading and writing CSV files
http://joshclose.github.io/CsvHelper/
Other
4.63k stars 1.05k forks source link

MissingFieldFound Should Allow Nulls (?) #2268

Closed LanceBresslerDelaget closed 2 weeks ago

LanceBresslerDelaget commented 3 weeks ago

Describe the bug First of all, I'm quite pleased to see null analysis enabled on the library. Thanks for the effort on that!

A minor hiccup I encountered after upgrading: I noticed that a CsvConfiguration with a null MissingFieldFound initializer now produces a warning. AFAICT all implementations and references to MissingFieldFound do support null values.

It's possible that other configuration delegates have similar issues - I'm not as familiar with those/which ones would make sense to allow null.

To Reproduce

CsvConfiguration config = new(CultureInfo.InvariantCulture)
{
  // TODO: I think this should be allowed. Instead it produces
  // CS8625: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/nullable-warnings?f1url=%3FappId%3Droslyn%26k%3Dk(CS8625)#possible-null-assigned-to-a-nonnullable-reference
  MissingFieldFound = null
};

Expected behavior Would expect this to be allowed, with nothing done about missing fields.

Screenshots Happy to provide if any more context would help.

Additional context From a quick clone, looks like marking MissingFieldFound properties and fields as nullable in CsvConfiguration, IReaderConfiguration, and CsvReader would be sufficient.

Happy to submit a PR for this, but per contribution guidelines, wanted to open the discussion and confirm my assumptions first.

Thanks!

JoshClose commented 2 weeks ago

Yes, I see my tests use it. I turned on nullable on my tests and am going through that now.

JoshClose commented 2 weeks ago

Fixed in latest release.