Skip to content

Commit

Permalink
v5.0.2.1: should work with future versions of AR too
Browse files Browse the repository at this point in the history
The migrations path was finally set as a separate method in the official
migration code since AR 5.0.2.
  • Loading branch information
rosenfeld committed Mar 2, 2017
1 parent 4384fa4 commit 32c3e8c
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 23 deletions.
20 changes: 11 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,20 @@ You can specify the environment by setting the `db` environment variable:

## Versioning

The version follows ActiveRecord versions plus a patch version from our own. For instance, if
AR version is 4.0.1, this gem will be versioned 4.0.1.x with x starting in 0.
From ActiveRecord 5.0.2 and above you should use v5.0.2.1 or above. That version isn't locked to
ActiveRecord 5.0.2 as in previous versions, which means it should work fine with any ActiveRecord
version starting with 5.0.2.

## I can't find a release for the AR version we rely on
Up to ActiveRecord 5.0.1 we relied on overriding a method in the migration generator to support
custom paths for the migrations, so the version used to follow ActiveRecord versions plus a
patch version from our own. For instance, if AR version is 4.0.1, this gem would be versioned
4.0.1.x with x starting in 0.

We have to use pessimistic versioning because we rely on internal details of AR migrations in
order to override the migrations path. So far the internal implementation since 4.0.0.0 hasn't
changed, so if you're interested on another version with optimistic versioning dependency in
order to have more flexibility you should check out the [optimistic](../../tree/optimistic) branch.
## I can't find a release for the AR version we rely on

I've already tried to merge the required changes to AR itself a few times but after having my
pull requests ignored a few times without feedback I gave up.
We had to use pessimistic versioning up to 5.0.1.1 because we relied on internal details of AR
migrations in order to override the migrations path. The [optimistic](../../tree/optimistic)
branch should work with any previous version since ActiveRecord 4.0.0.

## Contributing

Expand Down
10 changes: 2 additions & 8 deletions active_record_migrations.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,7 @@ Gem::Specification.new do |spec|

spec.add_development_dependency "bundler"
spec.add_dependency "rake"
# spec.add_dependency "railties", "~> 5.0.1"
# spec.add_dependency "activerecord", "~> 5.0.1"
# We rely on a kind of monkey patch for allowing us to override the migrations path.
# So it's better to fix on an specific version of AR and check if the override of
# ActiveRecord::Generators::MigrationGenerator#create_migration_file is correct
# before upgrading AR dependency. See ARM::Generators::MigrationGenerator impl.
spec.add_dependency "railties", "5.0.1"
spec.add_dependency "activerecord", "5.0.1"
spec.add_dependency "railties", ">= 5.0.2"
spec.add_dependency "activerecord", ">= 5.0.2"
end

7 changes: 2 additions & 5 deletions lib/active_record_migrations/generators/migration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,8 @@ module Generators
class MigrationGenerator < ::ActiveRecord::Generators::MigrationGenerator
source_root ::ActiveRecord::Generators::MigrationGenerator.source_root

def create_migration_file

This comment has been minimized.

Copy link
@ioquatix

ioquatix Mar 7, 2017

If you retain this function, like so https://github.com/ioquatix/activerecord-migrations/blob/master/lib/active_record/migrations/generator.rb you can work all current versions of AR ~> 5.0

However, it's possible it breaks in the future. As it is with any code..

This comment has been minimized.

Copy link
@rosenfeld

rosenfeld Mar 7, 2017

Author Owner

Yes, it is possible, but I think it's very unlikely they would remove or rename this method or change its meaning, so I'm comfortable with relaxing the versioning from now on and release a fix version in case things break in the future. Ideally, the ActiveRecord migrations should be a separate project from ActiveRecord. It's a shame we have to depend on railties just to be able to use the generators without rewriting a lot of code. Maybe the Rails core team could extract the generators code to a separate gem.

While keeping the previous method would support AR >= 4.0, the main reason for this change is that I didn't want to override that method in the first place. So, if we wanted to support all previous AR up to 4.0, we'd have to conditionally add that method for AR versions below 5.0.2. I'm just unsure on whether supporting all AR versions would be that useful. I can't think of any scenario where someone would want to use older AR with AR Migrations 5.0.2.1. So maybe it would be just some extra code to read and maintain.

set_local_assigns!
validate_file_name!
dir = ::ActiveRecord::Tasks::DatabaseTasks.migrations_paths.first
migration_template @migration_template, "#{dir}/#{file_name}.rb"
def db_migrate_path
::ActiveRecord::Tasks::DatabaseTasks.migrations_paths.first
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/active_record_migrations/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module ActiveRecordMigrations
VERSION = "5.0.1.1"
VERSION = "5.0.2.1"
end

4 comments on commit 32c3e8c

@ioquatix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you solved this. It's annoying that an API like this changed on a patch release.

@rosenfeld
Copy link
Owner Author

@rosenfeld rosenfeld commented on 32c3e8c Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that it's not really a public API. That's one of the main reasons I wasn't comfortable with patching the generator so that we could use the migrations path from the configurations. This method could change on any patch release since it's an implementation detail rather than a public API. I tried once to submit a patch to extract the path to a separate method years ago when I created this gem, but it was ignored if I recall correctly. I was happy to see that someone committed that change to Rails recently in rails/rails#27674

5.0.2 was the first release with this change included. I still have to override a private method, however it's very unlikely that someone would rename or remove the new db_migrate_path method, so I feel confident enough to relax on the versioning requirements for ActiveRecord. I was mostly worried about overriding create_migration_file, as it is a critical method with lots of behavior, and I always had to check on whether the implementation for it has changed before each new release.

@ioquatix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I hear what you are saying, based on my experience with AR, it's probably going to break again in the future :D

@rosenfeld
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that this change didn't actually break anything since this was an internal implementation detail. Nothing has changed in public APIs. I understand you don't appreciate incompatibilities introduced often by ActiveRecord, but they don't actually affect me because I don't actually use ActiveRecord as an ORM. I just use its migrations framework and use Sequel as the ORM. That means any incompatibilities in AR won't affect our application, except if they affect the migrations framework and so far I didn't see any changes in this framework that would cause any incompatibilities...

Please sign in to comment.