search

kbilsted / StatePrinter

Automating unit testing and ToString() coding
Apache License 2.0
93 stars 32 forks source link

Added caching for the configuration #60

Closed harrybellamy closed 5 years ago

harrybellamy commented 5 years ago

Added caching for the configuration in a generic class (see issue #37).

kbilsted commented 5 years ago

Many Thanks for the PR.

There are a few things I think need a bit of tweaking if you don't mind.

  1. Too many new namespaces - it doesn't follow the current style of the code
  2. the use of #region - it doesn't follow the current style of the code
  3. a get() that mutates state
  4. an interface abstraction for coincidental method name
harrybellamy commented 5 years ago

I've updated the PR based off your suggestions - removed the new classes and added new logic to Configuration.cs. I have spotted a potential bug though that was already present for the converter lookup implementation - if a ValueConverter is looked up for a type then a new one is added for the same type, the first implementation will still be returned from TryGetValueConverter.

I can look at fixing this in this PR if desired.

kbilsted commented 5 years ago

Hi thanks for reporting.

I was thinking, the bug scenario fix is just as bad as the bug? Since for the type T at some point one converter is used, then later, another converter is used without any warning or transparency.

kbilsted commented 5 years ago

regarding the cache class, try using a func as parameter for the ctor rather than a common interface. Perhaps in a new branch so as not to keep too much clutter around.

harrybellamy commented 5 years ago

I've backed out the cache class and replaced with a simple dictionary lookup in Configuration.cs.

harrybellamy commented 5 years ago

Closing as I should've branched from master for this.