Newtonsoft.Json icon indicating copy to clipboard operation
Newtonsoft.Json copied to clipboard

Separate StringEnumConverter into abstract base and specific implementation

Open Brondahl opened this issue 4 years ago • 2 comments

TL;DR

Makes it much easier to write a Custom StringEnumConverter. Has no external impact on existing class. (except an Exception message clarification; happy to revert is not wanted) Tests all pass.

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=

StringEnumConverter currently contains 2 separate kinds of code:

  • a Bunch of code that does all of the generalised interaction with the JSON infrastructure; interpretting the JsonReader and tokens, and so-forth
  • a Bunch of code relating to the specific details of how a specific enum value should be converted to a specific string, and vice versa.

If one wished to write a Converter that has a different opinion about how to do the actual conversion between a string and an enum, you'd have to copy-paste the two-thirds of the existing class, in order to provide your own implementation of the actual conversion. (You'll be shocked to learn that this is something I would like to do :) )

This PR, introduces an abstract Base class that encapsulates all of the 'boilerplate' and provides 2 abstract methods that are responsible for the actual conversions. The existing StringEumConverter then becomes an implementation of that abstract class, with no changes whatsoever. But if one wants to create CustomStringEnumConverter, then all you have to do is provide implementations of the actual string <-> enum conversions ... not worry about all of the other boilerplate.

Brondahl avatar May 25 '20 09:05 Brondahl

@JamesNK I was hoping to make use of the above for a project I'm currently working on. Do you have a feel for when you might be able to look at this PR, and accept it into the codebase?

(It's a pretty small code change, with no functional or NFR effects, and fully covered by existing Unit Tests :) )

Brondahl avatar Jun 15 '20 16:06 Brondahl

I think the check failure is spurious. The Logs have "Exception: Testhost process exited with error: It was not possible to find any compatible framework version" and the change that @sungam3r proposed is utterly trivial.

Is there some way to force a re-run?

Brondahl avatar Jul 09 '20 05:07 Brondahl