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

allow for arrays to be defined as symbol #13146

Closed
wants to merge 1 commit into from

Conversation

jigfox
Copy link
Contributor

@jigfox 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.

`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
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

jigfox commented Dec 3, 2013

@senny Thanks, that was pretty fast

senny added a commit that referenced this pull request Dec 3, 2013
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
Copy link
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 pushed a commit to fluxusfrequency/rails that referenced this pull request Dec 4, 2013
Closes rails#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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants