Avoid work in a initializer for IntegerMigrator

[sergio-fry]

Before run, migrator does a lot of work in an initializer. Initializing object should be a cheap operation. Let's avoid any work in there.

[jeremyevans]

Can you explain why you would want this? Are you creating IntegerMigrator objects without using them? Is this a style issue, are you trying to optimize something currently slow, or is there another reason you believe we should make this change?

I disagree that "Initializing object should be a cheap operation." is true in general. There are cases where the statement is true (e.g. Sequel::Model instances), but this doesn't appear to be one of those cases, so I'm hoping you explain the reasoning here.

[sergio-fry]

I am trying to build Rollback solution allowing to work both for IntegerMigrator and TimestampMigrator. It has interface:

    rollback = Sequel::Rollback.new(db, migrations_dir)
    rollback.run

It is expected to rollback the last applied migration, just like rails db:rollback. Class Sequel::Rollback is a facade for these migrator internals.

Actually, I am going to contribute this approach. But this PR is my test one, to check how difficult it is to contribute to sequel actually.

I can't say, that I have real performance issues, but I believe, code in contructor is common anti-pattern:

It’s a bad idea for one reason: It prevents composition of objects and makes them un-extensible.
https://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html

That's why I picked up this case to fix.

PS Thanks for quick response!

[jeremyevans]

I am trying to build Rollback solution allowing to work both for IntegerMigrator and TimestampMigrator. It has interface:

    rollback = Sequel::Rollback.new(db, migrations_dir)
    rollback.run

It is expected to rollback the last applied migration, just like rails db:rollback. Class Sequel::Rollback is a facade for these migrator internals.

This would work for IntegerMigrator, since when using IntegerMigrator, you can be sure in which order migrations were applied. You cannot be sure in which order the TimestampMigrator applied migrations. When using TimestampMigrator, the migration with the highest version is not necessarily the one most recently applied, and Sequel does not keep track of when migrations were applied. You could assume that the migration with the highest version is the one most recently applied, and that would be correct most of the time, but not all of the time.

Note that for IntegerMigrator, what you want is already supported by the relative: -1 option, there is no need to build a separate class for the feature.

Actually, I am going to contribute this approach. But this PR is my test one, to check how difficult it is to contribute to sequel actually.

I can't say, that I have real performance issues, but I believe, code in contructor is common anti-pattern:

It’s a bad idea for one reason: It prevents composition of objects and makes them un-extensible.
https://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html

I do not know Java well, maybe the problem the author is describing is a significant issue in Java. However, Java is not Ruby, and I don't think having code in constructors is a problem in Ruby. There are situations where you want to avoid it, to keep object initialization fast, as Sequel::Model does. That's because it's common to create hundreds or thousands of Sequel::Model instances per request and not sure most of the API per instance. For the migrators, usually there is one created per process, and doing the work in the constructor is simpler and results in more readable code.

I'm going to reject this as I don't think it improves the code. Hopefully this does not discourage you from contributing to Sequel in the future.

[sergio-fry]

OK.

Yes, you are write about order for TimestampMigrations, but it is often used in development, when you want to rollback last created. It works great for rails. Also even you don’t know the exact order, rollback should work anyways because these migrations were coded in independent features.

I think it would be nice to have
‘relative: -1 ‘ for TimestampMigrator as well. Don’t you think so?

[jeremyevans]

Sequel 5.75.0 will be released in a couple days, and will support TimestampMigrator.run_single for running a single migration up or down. :relative will not be supported by the TimestampMigrator, because there is no guarantee that the migration with the highest number is the most recently ran. Maybe Rails makes that assumption, and it's often correct, but IMO, it's a footgun. You can use the :target option to migrate to a given target version, which will migrate other migrations up or down as needed to reach the target version.

[sergio-fry]

:relative will not be supported by the TimestampMigrator, because there is no guarantee that the migration with the highest number is the most recently ran.

We can't guarantee the order when we use :target as well. Could you explain, why it is not a "footgun" for you?

[jeremyevans]

With :target, the user's intention is to migrate to a specific version, and Sequel migrates to that version, possibly running some migrations up and other migrations down as needed. This should be what the user expects, so I don't consider it a footgun.

Withrelative: -1, the user's intention is to rollback the most recently applied migration, but that cannot be guaranteed when using TimestampMigrator. If the most recently applied migration does not have the highest migration number, then this will migrate against the user's expectation, which is why I call it a footgun.

The entire reason for TimestampMigrator's existence is to be able to apply migrations out of order. If you only deal with in-order migrations, then the IntegerMigrator is a much better choice.