mysterioustrousers / MTMigration

Manages blocks of code that only need to run once on version updates in iOS apps.
BSD 2-Clause "Simplified" License
914 stars 50 forks source link

Gave some love to those blocks #13

Open ChocoChipset opened 10 years ago

ChocoChipset commented 10 years ago

Gave some love to those blocks, basically:

pwightman commented 10 years ago

Nice! Regarding tweet, all is well, loving the polish! I agree not crashing is wise, but can you maybe print a warning (if in DEBUG) that a migration was attempted with no block?

Also, I don't love that the side-effect of incrementing the migration version happens even with a nil block passed... But I suppose the name migrateToVersion would imply that side-effect, nil block or not, so maybe that's best.

Just thinking out loud, thoughts?

ChocoChipset commented 10 years ago

First I loved the idea of the warning. Then I noticed how convoluted the code became and didn't liked it that much. Also the side-effect kept me thinking. Check it out:

        if (migrationBlock) {   // avoid crash if migrationBlock turns out to be nil:

            migrationBlock();

            #if DEBUG
                NSLog(@"MTMigration: Running migration for version %@", version);
            #endif
        }
        else {
            #if DEBUG
                NSLog(@"MTMigration [Warning]: Running migration (%@) with empty execution block", version);
            #endif
        }

        [self setLastMigrationVersion:version];

Not sure then if the nil crash prevention is something to keep, since it wouldn't make sense to execute the code sending nil. However, would prevent crash in some actual cases (the block is a property of an object and this property was deallocated, hence becoming nil). I loved this library because of its simplicity and wouldn't like to hurt it by making it unnecessarily complicated.

Your call. :)

pwightman commented 10 years ago

Hmmm... Let me think on it a day or two. I like the idea of the warning, but if we're putting in that conditional logic, then we probably want it abstracted out into its own method so that both migrateToVersion:block: and applicationUpdateBlock: aren't duplicating logic... but they also probably need their own custom error messages... bleh. Let me think on it some.