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

Update column migration method to correctly set null: false #121

Merged
merged 2 commits into from
Jun 7, 2014
Merged

Update column migration method to correctly set null: false #121

merged 2 commits into from
Jun 7, 2014

Conversation

srt32
Copy link
Member

@srt32 srt32 commented Jun 6, 2014

  • Update MainAdapter#create_table to call add_spatial_column

Edit: Here is more specific information (taken from a comment that is now deleted):

Previously running

create_table :my_table do |t|
  t.column :geometry, :geometry, null: false
end

would produce a table with the geometry column without null: false. With this update, including the null: false flag will now set the column to be null: false as expected.

Simon Taranto added 2 commits June 6, 2014 11:17
  * Update `MainAdapter#create_table` to execute `change_column_null` if
    the column is set to null: false
@teeparham
Copy link
Member

This is great. Thanks.

The add_spatial_column signature is confusing, though:

def add_spatial_column(column_name, table_name, info = {}, type = nil, options)

# called by:
add_spatial_column(column_name, table_name, options)

While this works, it's confusing in that the info and type arguments get their default value, and the options argument maps to options. That is weird since the 3rd argument in the method call is mapped to the 5th argument in the signature.

I think the right solution to that is to combine the info and options hashes as the last argument & move the type munging into add_column. Any thoughts about that?

(I suppose I can merge this and we can fix it as a separate PR... )

@srt32
Copy link
Member Author

srt32 commented Jun 7, 2014

I agree we should clean up the signature and combine info and options (and
maybe remove type from options and require it as an input). I would vote
for doing that in a separate PR as I consider this PR a bug fix and the
other as more of a refactoring. I'm fine to fix it up in here if you prefer
though.

On Saturday, June 7, 2014, Tee Parham notifications@github.com wrote:

This is great. Thanks.

The add_spatial_column signature is confusing, though:

def add_spatial_column(column_name, table_name, info = {}, type = nil, options)

called by:

add_spatial_column(column_name, table_name, options)

While this works, it's confusing in that the info and type arguments get
their default value, and the options argument maps to options. That is
weird since the 3rd argument in the method call is mapped to the 5th
argument in the signature.

I think the right solution to that is to combine the info and options
hashes as the last argument & move the type munging into add_column. Any
thoughts about that?

(I suppose I can merge this and we can fix it as a separate PR... )


Reply to this email directly or view it on GitHub
#121 (comment)
.

teeparham added a commit that referenced this pull request Jun 7, 2014
Update column migration method to correctly set null: false
@teeparham teeparham merged commit c4cf6c2 into rgeo:master Jun 7, 2014
@teeparham
Copy link
Member

Yep, let's clean up the add_spatial_column method in a separate PR.

@srt32
Copy link
Member Author

srt32 commented Jun 7, 2014

Sounds good. I should be able to take a stab at it this week.

On Saturday, June 7, 2014, Tee Parham notifications@github.com wrote:

Yep, let's clean up the add_spatial_column method in a separate PR.


Reply to this email directly or view it on GitHub
#121 (comment)
.

@teeparham
Copy link
Member

Great - as always, check the latest master first since I may hack on it some too.

@jeremieweldin
Copy link

Hey guys, still not sure how to use this. How would I use add_spatial_column to add a polygon column using srid 4326?

@teeparham
Copy link
Member

@jeremieweldin this is an old, closed issue that applies to the internals of the adapter. Check the readme for version 2 or 3, depending on which version of rails you're using. There are examples for adding polygon columns there.

@jeremieweldin
Copy link

Sorry about that... limited results from the google search. :/ I will look into the readme. Thanks!

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.

3 participants