-
Notifications
You must be signed in to change notification settings - Fork 224
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
Support Attributes #334
Support Attributes #334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some todos left on README, and history to update. I've also made some comments, mostly questions to make sure I understood the code correctly.
def initialize(oid, sql_type) | ||
@sql_type = sql_type | ||
@geo_type, @srid, @has_z, @has_m = self.class.parse_sql_type(sql_type) | ||
def initialize(geo_type: 'geometry', srid: 0, has_z: false, has_m: false, geographic: false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should be updated as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point I'll do that!
# TODO: The attributes that will be joined have to be defined on the | ||
# model we make the query with. Ideally, it would "just work" but | ||
# at least this workaround makes joining functional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you plan anything for that ? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open up an issue once this is approved 😅
…stgres types under postgis
…forms type lookups on :postgis, then falls back to :postgresql
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks great, I added one last review, tell me what you think about it. Otherwise we'll be ready to merge soon 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work, thank you 💪
This adds support for the standard Postgresql attributes and RGeo features as attributes.
Changes:
postgresql
in thetype
module that is used for determining which types ActiveRecord should use. This fixes Unknown type Jsonb when using Attributes API. #267 and Unable to create an attribute for an array #284. Now users won't have to redefine Postgres attributes if they want them available in their app, but the downside is that if custom types were registered underpostgis
they will have to be changed topostgresql
.OID::Spatial
module to accept keywords as arguments (geo_type
,srid
,has_z
,has_m
,geographic
) instead ofoid
andsql_type
. This is a necessary step to allow RGeo features as attributes because attributes access the type directly and do not pass asql_type
in.initialize_type_map
to parsesql_type
then define the OID for each column.OID::Spatial
.With these changes, we can define attributes like this:
and it doesn't need an associated column in the database.
Importantly, this allows for the proper joining of spatial data (#210), although it is not seamless yet. In order to properly format RGeo features from a join, the spatial column needs to be re-defined as an attribute on the target table. For example:
TODO: