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

First steps towards AR 5 support on Postgres #12

Merged
merged 3 commits into from
Jun 10, 2016
Merged

First steps towards AR 5 support on Postgres #12

merged 3 commits into from
Jun 10, 2016

Conversation

md5
Copy link
Contributor

@md5 md5 commented Jun 8, 2016

Also includes an update to the version range to limit to AR < 6 (cf. SchemaPlus/schema_monkey#11) as well as some updates to the specs that relate to the order that AR 5 returns columns for polymorphic references.

@md5 md5 mentioned this pull request Jun 8, 2016
@coveralls
Copy link

coveralls commented Jun 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling b89016a on md5:activerecord-5.0 into fa201bb on SchemaPlus:activerecord-5.0.

@coveralls
Copy link

coveralls commented Jun 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 30b2f09 on md5:activerecord-5.0 into fa201bb on SchemaPlus:activerecord-5.0.

@md5
Copy link
Contributor Author

md5 commented Jun 8, 2016

Most of the failures for Postgres look like this:

Failure/Error: ActiveRecord::SchemaDumper.dump(ActiveRecord::Base.connection, stream)
     NoMethodError:
       undefined method `[]' for nil:NilClass
     # ./lib/schema_plus/core/active_record/schema_dumper.rb:78:in `block (2 levels) in table'
     # ./lib/schema_plus/core/active_record/schema_dumper.rb:69:in `map'

Those are caused by the inline t.index calls in create_table that I mentioned in #9

@coveralls
Copy link

coveralls commented Jun 9, 2016

Coverage Status

Coverage decreased (-0.2%) to 99.752% when pulling 180875f on md5:activerecord-5.0 into fa201bb on SchemaPlus:activerecord-5.0.

SchemaDump::Table::Column.new name: m[:name], type: m[:type], options: eval("{" + m[:options] + "}"), comments: []
}
if m.nil?
env.table.statements << statement
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the t.index statement will actually need to be parsed and turned into a SchemaDump::Table::Index instance (or something similar that outputs a new t.index string). Not sure whether it makes sense to add it to env.table.statements or have something like env.table.indexes.

@ronen
Copy link
Member

ronen commented Jun 9, 2016

The prepare_column_options fix looks fine of course. And I'm happy to accept it (despite the stil-failing build, since we can't expect any one PR to fix all the problems). But your other commits are all getting mingled in; especially 180875f. For cleanliness (a little bit for the commit history but mostly for the discussions) maybe make separate branches & PRs for each?

Regarding t.indexes: There actually already is env.table.indexes, see https://github.com/SchemaPlus/schema_plus_core/blob/970bf76/lib/schema_plus/core/active_record/schema_dumper.rb#L88-L100. So instead of adding m[:statements] I'd add m[:indexes]. But maybe move this discussion to a separate issue/PR?

Thanks again!

@md5
Copy link
Contributor Author

md5 commented Jun 9, 2016

@ronen Yeah, I got carried away trying to get the tests passing. Do you mind 427ab39 being in this PR?

@md5
Copy link
Contributor Author

md5 commented Jun 9, 2016

Or 30b2f09 for that matter

@md5
Copy link
Contributor Author

md5 commented Jun 9, 2016

I've dropped 180875f from this PR

I'll open an issue to discuss indexes_in_create separately.

@ronen
Copy link
Member

ronen commented Jun 10, 2016

In some highly principled world I suppose 427ab39 and 30b2f09 would be separate PRs. But they don't really need any discussion so let's just pretend this PR is called "first steps towards AR 5 support" and merge 'em :)

@ronen ronen merged commit a5310a6 into SchemaPlus:activerecord-5.0 Jun 10, 2016
@md5 md5 deleted the activerecord-5.0 branch June 11, 2016 05:53
@md5 md5 changed the title Update prepare_column_options to work with AR 5 First steps towards AR 5 support on Postgres Jun 11, 2016
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.

4 participants