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

Prefer SQL column type over normalized AR type #231

Merged
merged 1 commit into from
Apr 13, 2015
Merged

Prefer SQL column type over normalized AR type #231

merged 1 commit into from
Apr 13, 2015

Conversation

vassilevsky
Copy link
Contributor

We have columns of type point in our PostgreSQL database:

#<ActiveRecord::ConnectionAdapters::PostgreSQLColumn:0x007ffd7b829aa8
 @coder=nil,
 @default=nil,
 @limit=nil,
 @name="point",
 @null=false,
 @precision=nil,
 @primary=false,
 @scale=nil,
 @sql_type="point",
 @type=:string>

http://www.postgresql.org/docs/9.4/interactive/datatype-geometric.html

They are annotated as string in our models. This is misleading: point is actually 2 floats; not even close to a string.

I thought that SQL type should take precedence.

I understand that this change may be not enough or is completely wrong, and would welcome suggestions on the best solution.

@ctran
Copy link
Owner

ctran commented Mar 2, 2015

A small change but with big impact. I'm not sure how other would like this.

@vassilevsky
Copy link
Contributor Author

I think that people will like more correct annotations.

@alexch
Copy link
Collaborator

alexch commented Mar 17, 2015 via email

@ctran
Copy link
Owner

ctran commented Apr 10, 2015

What I meant is I don't know much this will change over the existing annotations. @alexch's suggestion make sense if you don't mind making that change.

@vassilevsky
Copy link
Contributor Author

I tested it on Rails 4.2.1 now.

rails new maps -d postgresql -T
cd maps
rails g model marker name location:point
class CreateMarkers < ActiveRecord::Migration
  def change
    create_table :markers do |t|
      t.string :name
      t.point :location

      t.timestamps null: false
    end
  end
end
rake db:create db:migrate
pgcli maps_development 
maps_development> \d markers
+------------+-----------------------------+------------------------------------------------------+
| Column     | Type                        | Modifiers                                            |
|------------+-----------------------------+------------------------------------------------------|
| id         | integer                     | not null default nextval('markers_id_seq'::regclass) |
| name       | character varying           |                                                      |
| location   | point                       |                                                      |
| created_at | timestamp without time zone | not null                                             |
| updated_at | timestamp without time zone | not null                                             |
+------------+-----------------------------+------------------------------------------------------+
# ...

ActiveRecord::Schema.define(version: 20150412093810) do

  # These are extensions that must be enabled in order to support this database
  enable_extension "plpgsql"

  create_table "markers", force: :cascade do |t|
    t.string   "name"
    t.point    "location"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
  end

end

I think that the proposed change will not cause much trouble in people's model annotations. First of all, not many people use those exotic types :) And if they do, and their strings change to points, then, after a brief wtf moment, they should feel better informed and therefore happy. Right?

ctran added a commit that referenced this pull request Apr 13, 2015
Prefer SQL column type over normalized AR type
@ctran ctran merged commit 8aa0be7 into ctran:develop Apr 13, 2015
@ctran ctran self-assigned this Apr 13, 2015
@ctran ctran added this to the v2.6.9 milestone Apr 13, 2015
@ctran ctran added the feature label Apr 13, 2015
@vassilevsky
Copy link
Contributor Author

Thank you.

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

Successfully merging this pull request may close these issues.

3 participants