search

JoshClose / CsvHelper

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

Create DefaultTypeConverterBase #1307

Open jzabroski opened 5 years ago

jzabroski commented 5 years ago

Is your feature request related to a problem? Please describe. When using ReSharper, I would like to type .TypeConverter<ClassNotYetImplementedTypeConverter>(); and hit control+. and have it suggest implementing DefaultTypeConverterBase instead of ITypeConverter. Even if ReSharper does not support this refactoring, I think it is valid to implement only one side of the DefaultTypeConverter.

Describe the solution you'd like

  1. Make DefaultTypeConverter sealed, have it implement DefaultTypeConverterBase
  2. Push-up method implementations from DefaultTypeConverter into DefaultTypeConverterBase
  3. Include solution as a major version upgrade, as it might be a breaking change for some clients.

Describe alternatives you've considered Add new faceted type converters API - a FromTypeConverter API and ToTypeConverter API.

Additional context I logged a bug with JetBrains ReSharper here: https://youtrack.jetbrains.com/issue/RSRP-474706

However, it was in the course of logging the feature with JetBrains that I realized there was no ABC for DefaultTypeConverter.

JoshClose commented 5 years ago

What do you mean by

no ABC for DefaultTypeConverter.

jzabroski commented 5 years ago

ABC = Abstract Base Class. DefaultTypeConverter should be sealed, and it's methods should be pushed-up into an abstract DefaultTypeConverterBase. My guess is this is about 5 minutes of work, but it is a breaking change.

JoshClose commented 5 years ago

Ha, gotcha.

I've been trying to not make anything sealed and make everything virtual so people can override anything they want.

JoshClose commented 5 years ago

I'm not opposed to making a TypeConverterBase class and doing your suggestion though.

jzabroski commented 5 years ago

The reason to make your default implementation sealed is so that your "core runtime" has a default implementation it can reference. The reason for creating an ABC above that is tooling. This way we have our cake and eat it too. I'll submit a PR for you to review; thanks for the general feedback that this is OK and not going to upset anything.

jzabroski commented 5 years ago

@JoshClose I'll submit a PR next week when I'm on vacation.