search

dradovic / MigSharp

Mig# (MigSharp) is a .NET framework for database migrations and versioning
Other
106 stars 34 forks source link

Support for custom timestamp format #43

Closed richardprior closed 11 years ago

richardprior commented 11 years ago

Adds a IMigrationTimestampProvider property called TimestampProvider to MigrationOpions. This allows the user to customise how they fetch the timestamp from the migration. The current method of post-fixing the timestamp is included in the MigSharp project. Implementions of attribute-based timestamp retrieval and interface-based timestamp retrieval are included in the tests.

richardprior commented 11 years ago

Additionally, I see there's already a pull request for a custom timestamp formatter but it was over a year old and I didn't agree with the need for a migration requiring a base class, it seems more of a configuration option to me - hence the addition to MigrationOptions.

dradovic commented 11 years ago

Thanks for your pull request. Having a customizable way of specifying timestamps seems to be an important feature.

I like your approach, except that having an IMigrationTimestampProvider as a property on the MigrationOptions doesn't help for the console app. When you launch Mig# from the command line, you cannot pass object instances - only text. So my preferred solution to this would be to use MEF as suggested in the discussion of #31. In this way, the client doesn't need to set the IMigrationTimestampProvider explicitly and it works for the console app as well. What do you think about that?

richardprior commented 11 years ago

That's a good point, I don't use the console application so missed that. I've updated the time stamp provider to be loaded by MEF and added an export attribute to support multi-modules.

The load priority of time stamp providers is now:

  1. Time stamp provider exported from assembly with matching module name
  2. Time stamp provider exported from assembly with default module name
  3. Default time stamp provider from MigSharp
dradovic commented 11 years ago

From the first glance it looks like you did a fabulous job here! This will be definitely included in the next version of Mig#. Currently, I'm busy working on another project but I think I will get to it mid December.

Remaining questions from the top of my head:

  1. For you or me: Raise error, if more than one TimestampProvider is defined for the same module (to prevent tricky bug finding).
  2. To myself: the documentation needs to be updated to describe this new feature.
richardprior commented 11 years ago

I've updated the code to raise an error if more than one TimestampProvider is defined for the same module as well as adding some tests to support this.

I didn't want to edit your wiki directly, but I've also put up some initial documentation (https://github.com/richardprior/MigSharp/wiki/Timestamp-Documentation-Update) which should replace the 'Specifying a Timestamp (Version) of a Migration' section of your manual. I've tried to keep it in the same style as the rest of your documentation so hopefully it should just be a case of cutting and pasting.

(Thanks for taking the time to create and maintain this project by the way, your efforts are most appreciated!)

dradovic commented 11 years ago

Wow - excellent work! Can't wait to integrate it! :+1:

richardprior commented 11 years ago

I have a confession to make...I hadn't set up the environment for all the DB specifc tests so only ran the main NUnit project. When investigating another issue which required me running these tests I realised I'd broken the integration tests by exporting intentionally broken timestamp providers in the main test assembly. I've updated the test code so that the migrations and providers used for the timestamp provider tests are compiled into an in-memory assembly and used from there.

I also made an incorrect assumption about the way you were loading migrations. I'd assumed that if a module was specified that the migrations would be filtered on load. It appears that you are loading all the migrations and filtering later so the way the timestamp provider was implemented did not work in all scenarios. I've fixed this as well. All tests are now green :-) (Disclaimer: Haven't run Oracle/Teradata test suites)

dradovic commented 11 years ago

I've just finished integrating your changes. Excellent work! Thanks a lot!

richardprior commented 11 years ago

No worries, glad I could help. On 5 Dec 2012 15:02, "dradovic" notifications@github.com wrote:

I've just finished integrating your changes. Excellent work! Thanks a lot!

— Reply to this email directly or view it on GitHubhttps://github.com/dradovic/MigSharp/pull/43#issuecomment-11044476.