chucknorris / roundhouse

RoundhousE is a Database Migration Utility for .NET using sql files and versioning based on source control
http://projectroundhouse.org
920 stars 247 forks source link

Move token dictionary creation to ConfigurationPropertyHolder #329

Closed chuseman closed 6 years ago

chuseman commented 6 years ago

This will move the creation of the token dictionary out of TokenReplacer and into the ConfigurationPropertyHolder implementation. Along the way, #328 will be fixed.

A few questions:

  1. The roundhouse project has the C# language version set to 5 - I assume this is intentional but I'd just like to make sure. nameof() and collection initialization are unavailable in C# 5.
  2. I've dropped the DefaultEncoding and Logger properties from the dictionary - are there others that aren't appropriate? They're all primitive types that are at least readable as strings.
  3. The EnvironmentName key is set to the same value as EnvironmentNames (comma-delimited list of environment names) - should it just be the first environment? Or only present if there is a single environment?
  4. There are two implementations of ConfigurationPropertyHolder - is there any reason the roundhouse.tasks.Roundhouse implementation doesn't extend roundhouse.consoles.DefaultConfiguration instead of reimplementing the interface?
BiggerNoise commented 6 years ago

Thank you very much for this. To address your points.

  1. Intentional so that we can continue to support older versions of .NET. Eventually, we'll drop that support and upgrade our version. I am quite liking many features from C#7.
  2. I agree with the decision to drop those. There probably are others that shouldn't be there, but I doubt they're hurting anything.
  3. I think you made the correct call here. There's no really good answer for a corner case like this.
  4. I have no idea why it is done that way. I would certainly use the implementation and extend it as needed. I'd really need to dig into that code further.
BiggerNoise commented 6 years ago

Fixes #328