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

schema dumper tests now conducted by ActiveRecord::Base.Connection #9204

Merged
merged 1 commit into from Feb 12, 2013

Conversation

ranjaykrishna
Copy link
Contributor

We abstracted the test for valid data types from databases from the schema dumper to Connection. We set it so that sqlite would approve all values. However, you can not create arbitrary types in sqlite as it still checks against its native_database_types.

@jaggederest
Copy link
Contributor

Updated patch looks good, but I don't think it requires releasing the Kraken just yet :) commit da19327 introduced a change that isn't otherwise used here.

@jaggederest
Copy link
Contributor

👍

@jeremy
Copy link
Member

jeremy commented Feb 8, 2013

👾

@carlosantoniodasilva
Copy link
Member

Please make sure you squash the commits :).

@ranjaykrishna
Copy link
Contributor Author

How do I squash the commits?

@carlosantoniodasilva
Copy link
Member

I think this post from @steveklabnik might help :)

@ranjaykrishna
Copy link
Contributor Author

@carlosantoniodasilva I think I have squashed the commits.

@steveklabnik
Copy link
Member

It still says there are 11: https://github.com/rails/rails/pull/9204/commits

Did you force push to the branch successfully?

@ranjaykrishna
Copy link
Contributor Author

I believe that the commits have now been successfully squashed here as well.

tenderlove added a commit that referenced this pull request Feb 12, 2013
schema dumper tests now conducted by ActiveRecord::Base.Connection
@tenderlove tenderlove merged commit f8c8ad5 into rails:master Feb 12, 2013
@rubys
Copy link
Contributor

rubys commented Feb 13, 2013

This change impacts the ability to create a db/schema.rb file. See:

f8c8ad5#L3R608

@kenips
Copy link

kenips commented Feb 13, 2013

Yes, seeing same error here.

@rubys
Copy link
Contributor

rubys commented Feb 13, 2013

I didn't draw the connection at the time, but I'm seeing lots of errors as a result of this:

http://intertwingly.net/projects/AWDwR4/checkdepot/

All go away if valid_type? is made public.

@ranjaykrishna
Copy link
Contributor Author

I just moved them to public methods. Running additional tests now.

@jonleighton
Copy link
Member

Hi @ranjaykrishna, I have reverted this commit because it introduced caused a failing test. I have also reverted the commit by @rubys as it relates solely to this commit. Please fix up the test and send a new PR incorporating the commit by @rubys also.

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

Successfully merging this pull request may close these issues.

None yet

10 participants