Support for Active Record 5 #4

myabc · 2016-08-14T22:18:28Z

You probably want to merge this into the activerecord-5.0 branch rather than master. PR is here for discussion/tracking (unlike Full support for AR5 schema_plus_core#14 though, it might not be so difficult to support AR 4.2 + 5.0 in the same project).

Signed-off-by: Alex Coles <alex@alexbcoles.com>

ronen · 2016-08-15T13:31:55Z

@myabc Thanks for working on this! Since you marked it as Work In Progress I'll ignore it for now until you say it's ready.

BTW I presume you're using the soon-to-be-released schema_plus_core 2.0.0 that @boazy created in SchemaPlus/schema_plus_core/pull/14 ?

myabc · 2016-08-15T14:41:31Z

@ronen I have specs passing locally for MySQL; PostgreSQL. SQLite3 still needs some work, hence the PR title (It's probably the RDBMS I'm least familiar with).

BTW I presume you're using the soon-to-be-released schema_plus_core 2.0.0 that @boazy created in SchemaPlus/schema_plus_core#14 ?

Having this in my Gemfile.local works fine:

gem 'schema_plus_core', github: 'boazy/schema_plus_core',
                        branch: 'activerecord-5.0'

I'm just trying to get the same working on Travis.

myabc · 2016-08-15T14:44:25Z

spec/command_recorder_spec.rb

@@ -37,7 +37,7 @@ def before(env)
    end

    it "does not fail when reverting" do
-      migration = Class.new ::ActiveRecord::Migration do
+      migration = Class.new ::ActiveRecord::Migration[5.0] do


this allows the spec to pass, but is probably not the right solution:

env.options[:index] = { name: 'whatever' } if enabled_middleware(AddIndexBefore, env)

~~(in this case, the call to enabled_middleware always returns false)~~

I need to better understand what the desired behaviour should be.

Strange that enabled_middleware always returns false. Are you sure? (Actually it should return either a class or nil. See spec/support/enableable.rb

The intent of the spec is to make sure that the middleware doesn't cause a :down migration to fail; I probably introduced this test to fix a bug. In particular, it should exercise the code in lib/schema_plus/indexes/active_record/migration/command_recorder.rb

Strange that enabled_middleware always returns false. Are you sure? (Actually it should return either a class or nil. See spec/support/enableable.rb

Sorry, I had meant falsey (i.e. nil), not false.

(in this case, the call to enabled_middleware always returns false)

This statement was actually incorrect. I took a fresh look at it this morning. All appending [5.0] to ::ActiveRecord::Migration does is fix a deprecation warning.

myabc · 2016-08-15T14:56:58Z

@ronen Do you have a standard way of telling Travis to use a different upstream branch. I had considered configuring .travis.yml to write out a `Gemfile.local, but I know it's auto-generated, thus would be overriden on the first matrix run.

coveralls · 2016-08-15T19:28:08Z

Coverage decreased (-3.9%) to 96.094% when pulling 7f02db6 on myabc:activerecord-5.0 into 8fe9ae7 on SchemaPlus:master.

coveralls · 2016-08-15T19:28:08Z

Coverage decreased (-3.9%) to 96.094% when pulling 7f02db6 on myabc:activerecord-5.0 into 8fe9ae7 on SchemaPlus:master.

ronen · 2016-08-15T22:51:06Z

@myabc

Do you have a standard way of telling Travis to use a different upstream branch.

The thing you did with Gemfile.local ought to work I think... but in any case it's moot since I've just released schema_plus_core 2.0.0

Signed-off-by: Alex Coles <alex@alexbcoles.com>

coveralls · 2016-08-16T08:19:19Z

Coverage remained the same at 100.0% when pulling 53e4c45 on myabc:activerecord-5.0 into 8fe9ae7 on SchemaPlus:master.

coveralls · 2016-08-16T08:24:41Z

Coverage remained the same at 100.0% when pulling f0b08df on myabc:activerecord-5.0 into 8fe9ae7 on SchemaPlus:master.

Currently the `Result` returned includes auto-generated indexes e.g. `sqlite_autoindex_ar_internal_metadata_1` for which the `sql` value will always be `nil`. Signed-off-by: Alex Coles <alex@alexbcoles.com>

In these cases, `#tables` is actually the appropriate method. This reverts commit e543c18.

myabc · 2016-08-16T08:47:53Z

lib/schema_plus/indexes/middleware/schema.rb

@@ -8,7 +8,7 @@ def after(env)
            indexes = Hash[env.index_definitions.map{ |d| [d.name, d] }]

            env.connection.exec_query("SELECT name, sql FROM sqlite_master WHERE type = 'index'").map do |row|
-              if (desc_columns = row['sql'].scan(/['"`]?(\w+)['"`]? DESC\b/).flatten).any?
+              if row['sql'] && (desc_columns = row['sql'].scan(/['"`]?(\w+)['"`]? DESC\b/).flatten).any?


@ronen I'm not completely sure why the query returns auto-generated indexes here.

I'm OK with adding the guard if the tests show it's needed.

myabc · 2016-08-16T08:48:11Z

@ronen I think this is now ready for an initial review.

coveralls · 2016-08-16T08:51:28Z

Coverage remained the same at 100.0% when pulling 02283eb on myabc:activerecord-5.0 into 8fe9ae7 on SchemaPlus:master.

ronen · 2016-08-17T09:16:37Z

.travis.yml

@@ -5,11 +5,11 @@
 ---
 sudo: false
 rvm:
- 2.1.5
+- 2.3.0


May as well use 2.3.1 , since that's the current officially stable release.

ronen · 2016-08-17T09:43:53Z

@myabc thanks again for working on this!

Duplicating my own comment in SchemaPlus/schema_plus_foreign_keys#11 (comment) ...

My only other overall question is, as AR keeps increasing its support for indexes, what (if any) functionality of schema_plus_indexes is now natively supported by AR? Though potentially with different syntax?

(Revealing my ignorance about AR 5, I don't know what has been added. Part of why I've been so behind the ball on the AR 5 upgrades to SchemaPlus is that I'm not currently using rails much in my day jobs.)

Signed-off-by: Alex Coles <alex@alexbcoles.com>

coveralls · 2016-08-17T22:04:46Z

Coverage remained the same at 100.0% when pulling 1b4cfee on myabc:activerecord-5.0 into 8fe9ae7 on SchemaPlus:master.

Signed-off-by: Alex Coles <alex@alexbcoles.com>

coveralls · 2016-08-18T17:42:17Z

Coverage remained the same at 100.0% when pulling 0c9d021 on myabc:activerecord-5.0 into 8fe9ae7 on SchemaPlus:master.

boazy · 2016-08-21T09:04:19Z

Is this PR ready to be merged?
Once it gets released, schema_plus_columns could be released as well.
Next comes schema_validations, but seems to take a little bit more work.

ronen · 2016-08-24T23:39:02Z

Sorry for the delay. @myabc thanks for the update!

I've merged and adjusted the dependencies because this actually still works for AR 4.2 and 5.0. Since there are no new features or breaking changes, this is just a patch release :)

I've released v0.2.4

myabc added 5 commits August 14, 2016 19:56

Update .gemspec to depend on schema_plus_core 2.0

6d5579a

Signed-off-by: Alex Coles <alex@alexbcoles.com>

Only test against ActiveRecord 5.0, Ruby 2.3.0

349fd7e

Update deprecated #tables to #data_sources

e543c18

Signed-off-by: Alex Coles <alex@alexbcoles.com>

Explicitly override PostgreSQLAdapter#remove_index

3587d52

Signed-off-by: Alex Coles <alex@alexbcoles.com>

Specify Migration version for CommandRecorder spec

95d51a0

Signed-off-by: Alex Coles <alex@alexbcoles.com>

myabc changed the title ~~Activerecord 5.0~~ Full support for Active Record 5 Aug 15, 2016

myabc changed the title ~~Full support for Active Record 5~~ Support for Active Record 5 Aug 15, 2016

myabc reviewed Aug 15, 2016
View reviewed changes

myabc force-pushed the activerecord-5.0 branch 2 times, most recently from 3f2d201 to 7f02db6 Compare August 15, 2016 19:25

myabc force-pushed the activerecord-5.0 branch from 7f02db6 to 95d51a0 Compare August 16, 2016 07:39

Explicitly override SQLite3Adapter#remove_index

91d97ce

Signed-off-by: Alex Coles <alex@alexbcoles.com>

myabc added 2 commits August 16, 2016 09:46

Add sanity check to Schema::SQLite3::Indexes

e5371fd

Currently the `Result` returned includes auto-generated indexes e.g. `sqlite_autoindex_ar_internal_metadata_1` for which the `sql` value will always be `nil`. Signed-off-by: Alex Coles <alex@alexbcoles.com>

Revert "Update deprecated #tables to #data_sources"

02283eb

In these cases, `#tables` is actually the appropriate method. This reverts commit e543c18.

myabc force-pushed the activerecord-5.0 branch from f0b08df to 02283eb Compare August 16, 2016 08:47

myabc reviewed Aug 16, 2016
View reviewed changes

ronen reviewed Aug 17, 2016
View reviewed changes

Bump tested Ruby to 2.3.1

1b4cfee

Signed-off-by: Alex Coles <alex@alexbcoles.com>

DRY up: Extract #remove_index override into Module

0c9d021

Signed-off-by: Alex Coles <alex@alexbcoles.com>

myabc mentioned this pull request Aug 19, 2016

Support for Active Record 5 SchemaPlus/schema_plus_pg_indexes#11

Closed

ronen merged commit ff0a3b6 into SchemaPlus:master Aug 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Active Record 5 #4

Support for Active Record 5 #4

myabc commented Aug 14, 2016 •

edited

Loading

ronen commented Aug 15, 2016

myabc commented Aug 15, 2016

myabc Aug 15, 2016 •

edited

Loading

ronen Aug 15, 2016

myabc Aug 16, 2016

myabc commented Aug 15, 2016

coveralls commented Aug 15, 2016

coveralls commented Aug 15, 2016

ronen commented Aug 15, 2016

coveralls commented Aug 16, 2016 •

edited

Loading

coveralls commented Aug 16, 2016 •

edited

Loading

myabc Aug 16, 2016

ronen Aug 17, 2016

myabc commented Aug 16, 2016

coveralls commented Aug 16, 2016 •

edited

Loading

ronen Aug 17, 2016

ronen commented Aug 17, 2016

coveralls commented Aug 17, 2016 •

edited

Loading

coveralls commented Aug 18, 2016 •

edited

Loading

boazy commented Aug 21, 2016

ronen commented Aug 24, 2016 •

edited

Loading

Support for Active Record 5 #4

Support for Active Record 5 #4

Conversation

myabc commented Aug 14, 2016 • edited Loading

ronen commented Aug 15, 2016

myabc commented Aug 15, 2016

myabc Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

ronen Aug 15, 2016

Choose a reason for hiding this comment

myabc Aug 16, 2016

Choose a reason for hiding this comment

myabc commented Aug 15, 2016

coveralls commented Aug 15, 2016

coveralls commented Aug 15, 2016

ronen commented Aug 15, 2016

coveralls commented Aug 16, 2016 • edited Loading

coveralls commented Aug 16, 2016 • edited Loading

myabc Aug 16, 2016

Choose a reason for hiding this comment

ronen Aug 17, 2016

Choose a reason for hiding this comment

myabc commented Aug 16, 2016

coveralls commented Aug 16, 2016 • edited Loading

ronen Aug 17, 2016

Choose a reason for hiding this comment

ronen commented Aug 17, 2016

coveralls commented Aug 17, 2016 • edited Loading

coveralls commented Aug 18, 2016 • edited Loading

boazy commented Aug 21, 2016

ronen commented Aug 24, 2016 • edited Loading

myabc commented Aug 14, 2016 •

edited

Loading

myabc Aug 15, 2016 •

edited

Loading

coveralls commented Aug 16, 2016 •

edited

Loading

coveralls commented Aug 16, 2016 •

edited

Loading

coveralls commented Aug 16, 2016 •

edited

Loading

coveralls commented Aug 17, 2016 •

edited

Loading

coveralls commented Aug 18, 2016 •

edited

Loading

ronen commented Aug 24, 2016 •

edited

Loading