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.

Sign up for GitHub

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

Jump to bottom

Use t.column instead of t.#{type} for database dependent types definition #114

Merged
merged 2 commits into from
Jun 2, 2017
Merged

Use t.column instead of t.#{type} for database dependent types definition #114

merged 2 commits into from
Jun 2, 2017

Conversation

aycabta
Copy link
Contributor

@aycabta aycabta commented May 31, 2017

In ActiveRecord, the documant of TableDefinition#column says

ConnectionAdapters::SchemaStatements#add_column for available options.

And the document of ConnectionAdapters::SchemaStatements#add_column says

The type parameter is normally one of the migrations native types, which is one of the following:
(snip)
You may use a type not in this list as long as it is supported by your database (for example, "polygon" in MySQL), but this will not be database agnostic and should usually be avoided.

So I may use some database dependent types, but Ridgepole spoils this feature because it uses t.#{column_type} for definition.

This Pull Request changes it with t.column. Ridgepole users may use database dependent types.

@aycabta aycabta changed the title Use t.column instead of t.#{type} for definition Use t.column instead of t.#{type} for database dependent types definition May 31, 2017
@aycabta
Copy link
Contributor Author

aycabta commented May 31, 2017

Please wait for fixing and adding test 😖

@coveralls
Copy link

coveralls commented May 31, 2017 •
edited
Loading

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

7 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38bc2df on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@aycabta
Copy link
Contributor Author

aycabta commented May 31, 2017 •
edited
Loading

Done, but this Pull Request needs #115 to pass specs before merging this.

@coveralls
Copy link

coveralls commented May 31, 2017 •
edited
Loading

Coverage Status

Changes Unknown when pulling 7d6287b on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7d6287b on aycabta:use-column-for-definition into ** on winebarrel:0.7**.

@winebarrel
Copy link
Member

I'm sorry...
Rebuilding now

@winebarrel winebarrel merged commit b0e5c98 into ridgepole:0.7 Jun 2, 2017
@winebarrel
Copy link
Member

winebarrel commented Jun 2, 2017 •
edited
Loading

Merged.
I am sorry to delay.

I will try to fix failed test.

I think it's a good fix :-)
Thank you!

@aycabta
Copy link
Contributor Author

aycabta commented Jun 2, 2017

Currently I am fixing a lot of tests for Rails 5.1.
So I think merge the pull request and fix it together.

I added tests for this Pull Request at 7d6287b, but I think it may become to be unnecessary for new test set of Ridgepole for ActiveRecord 5.1. OK, please merge after your works for 5.1 are finished.

I'm sorry...

DO NOT apologize. This is your software. You have all prerogatives for it. I wish you glory about all works.

By the way, I can give help for maintenance others. Please take me commi...

Merged.

Wooow. It's so early. Great works!

@aycabta
Copy link
Contributor Author

aycabta commented Jun 2, 2017

Can you merge branch 0.7 to master and release new 0.6.x? Maybe you can it now.

@winebarrel
Copy link
Member

Can you merge branch 0.7 to master and release new 0.6.x?

yes. backport it.

@aycabta aycabta deleted the use-column-for-definition branch June 2, 2017 12:36
@winebarrel winebarrel mentioned this pull request Jun 2, 2017
Merged
@winebarrel
Copy link
Member

0.6.6.beta has been released.
Please try it out :-)

@aycabta
Copy link
Contributor Author

aycabta commented Jun 2, 2017

OK! Thanks a lot!!

@aycabta
Copy link
Contributor Author

aycabta commented Jun 2, 2017

I succeeded in my product code, thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aycabta @coveralls @winebarrel