allow for arrays to be defined as symbol #13146

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@jigfox
Contributor

jigfox commented Dec 3, 2013

change_colum :table, :column, :bigint, array: true didn't work because
the array option tried to string concat: :bigint << '[]'

I don't know if this is the right place to convert the possible symbol
to a string. If it's wrong please let me know a better place and I will
change this.

allow for arrays to be defined as symbol
`change_colum :table, :column, :bigint, array: true` didn't work because
the array option tried to string concat: `:bigint << '[]'`

I don't know if this is the right place to convert the possible symbol
to a string. If it's wrong please let me know a better place and I will
change this.
@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 3, 2013

Member

@jigfox if you look at the test a few lines below it seems that symbols are already working...

@connection.change_column :pg_arrays, :snippets, :text, array: true, default: "{}"
Member

senny commented Dec 3, 2013

@jigfox if you look at the test a few lines below it seems that symbols are already working...

@connection.change_column :pg_arrays, :snippets, :text, array: true, default: "{}"
@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 3, 2013

Member

@jigfox I think you hit a special case with bigint. Looking at the implementation of type_to_sql I see that you should probably use :integer, limit: 8. See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L456

I'll investigate and let you know what I find out.

Member

senny commented Dec 3, 2013

@jigfox I think you hit a special case with bigint. Looking at the implementation of type_to_sql I see that you should probably use :integer, limit: 8. See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L456

I'll investigate and let you know what I find out.

@jigfox

This comment has been minimized.

Show comment Hide comment
@jigfox

jigfox Dec 3, 2013

Contributor

Okay, I just started writing that it seems to be a special case for :bigint

I know I could have used limit: 8, but IMHO it should work with bigint, too

Contributor

jigfox commented Dec 3, 2013

Okay, I just started writing that it seems to be a special case for :bigint

I know I could have used limit: 8, but IMHO it should work with bigint, too

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 3, 2013

Member

The docs say:

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.

The example uses a String but I agree that it should behave the same way if you use a Symbol. However patching change_column only in the PG adapter is at the wrong level. I'll look into it and prepare a patch.

Member

senny commented Dec 3, 2013

The docs say:

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.

The example uses a String but I agree that it should behave the same way if you use a Symbol. However patching change_column only in the PG adapter is at the wrong level. I'll look into it and prepare a patch.

@jigfox

This comment has been minimized.

Show comment Hide comment
@jigfox

jigfox Dec 3, 2013

Contributor

psotgres seems to be the only database to support the array option

Contributor

jigfox commented Dec 3, 2013

psotgres seems to be the only database to support the array option

@senny senny closed this in be5527b Dec 3, 2013

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 3, 2013

Member

@jigfox I commited be5527b to fix the problem. type_to_sql now has a consistent signature and will always return a String. This will allow dependent code like change_column to perform String operations like << on the result.

Thank you for reporting 💛

Member

senny commented Dec 3, 2013

@jigfox I commited be5527b to fix the problem. type_to_sql now has a consistent signature and will always return a String. This will allow dependent code like change_column to perform String operations like << on the result.

Thank you for reporting 💛

@jigfox

This comment has been minimized.

Show comment Hide comment
@jigfox

jigfox Dec 3, 2013

Contributor

@senny Thanks, that was pretty fast

Contributor

jigfox commented Dec 3, 2013

@senny Thanks, that was pretty fast

senny added a commit that referenced this pull request Dec 3, 2013

`connection.type_to_sql` returns a `String` for unmapped types.
Closes #13146.

This fixes an error when using:

```
change_colum :table, :column, :bigint, array: true
```

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/test/cases/adapter_test.rb
@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 3, 2013

Member

@jigfox you're welcome. I backported the fix to 4-0-stable to be released with the next 4.0.x release.

Member

senny commented Dec 3, 2013

@jigfox you're welcome. I backported the fix to 4-0-stable to be released with the next 4.0.x release.

fluxusfrequency added a commit to fluxusfrequency/rails that referenced this pull request Dec 4, 2013

`connection.type_to_sql` returns a `String` for unmapped types.
Closes #13146.

This fixes an error when using:

```
change_colum :table, :column, :bigint, array: true
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment