Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gave some love to those blocks #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChocoChipset
Copy link
Contributor

Gave some love to those blocks, basically:

  • 781e4dc: All the blocks use the same signature, I type-defed them to improve readability and consistency.
  • a6677b7: If you send nil to any of the block parameters, BAM!! 💣 An EXC_BAD_ACCESS error will happen and will cause the app to crash. Since they are public methods, I consider wise to guard them against possible scenarios where this might happen.

@pwightman
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants