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

[WIP] upgrade to rom-sql 1.0.0 #370

Closed
wants to merge 9 commits into from
Closed

Conversation

solnic
Copy link
Member

@solnic solnic commented Jan 11, 2017

TODO

  • figure out why tests are failing with attributes that failed to inferred (while the same thing happens with rom-sql 0.9.1, tests are passing regardless)
  • figure out why entities schema coercion behaves differently (thus a couple of tests are failing)
  • bump rom deps to released final versions (once we have them :))
  • fix remaining specs for postgresql and mysql

@jodosha jodosha modified the milestone: v1.0.0.beta1 Jan 13, 2017
@solnic solnic force-pushed the rom-sql-1.0 branch 2 times, most recently from 0329275 to f9601ba Compare January 19, 2017 13:13
@solnic
Copy link
Member Author

solnic commented Jan 21, 2017

@jodosha so, that's practically done, just waiting for final releases of rom gems. As you can see changes are tiny. I'm not 100% happy with schema mapping though. I don't have any immediate ideas how to make it work better. From what I've seen hanami entities are supposed to handle coercions both ways, I understand why you did it like that but it's not how rom works, as we have separated input/output coercions in rom 3.0.0. I don't want this to block us though, so let's just stick to what we have now and we can figure it out in hanami-model 2.0.0 (or maybe 1.x).

@solnic
Copy link
Member Author

solnic commented Jan 21, 2017

Uhm, not sure why travis is failing, tests pass on my machine for all dbs

@jodosha
Copy link
Member

jodosha commented Jan 21, 2017

@solnic It fails for me locally too. Please remember that when you run bundle exec rake it only run specs agains SQLite. If you want to run for another database you should do:

DB=sqlite bundle exec rake
# or
DB=mysql bundle exec rake
# or
DB=postgresql bundle exec rake

@solnic
Copy link
Member Author

solnic commented Jan 21, 2017

@jodosha oh damn :D ok I'll look into that :)

@solnic
Copy link
Member Author

solnic commented Jan 30, 2017

@jodosha tests are failing on postgres because inferrer doesn't support line, circle and especially custom-made types, like the inventory_item one. It passes on rom-sql 0.9.x probably because we are inferring some default type by an accident for these columns.

@flash-gordon is it even possible to infer custom-defined types?

@solnic
Copy link
Member Author

solnic commented Jan 30, 2017

Answering to myself: nope, because custom type has no info about any matching ruby type:

[:composite1, {:oid=>6663255, :db_type=>"inventory_item", :default=>"ROW('fuzzy dice'::text, 42, 1.99)", :allow_null=>true, :primary_key=>false, :type=>nil, :ruby_default=>nil}]

No idea what to do with that, except having ability to manually specify the attribute in schema

@flash-gordon
Copy link
Member

@solnic I didn't look deep, but I saw Ecto handles it, so it is possible in one way or another. Right now I can tell that Sequel has a weird API for composite types https://github.com/jeremyevans/sequel/blob/master/spec/adapters/postgres_spec.rb#L3469 I'll have a more thorough look into that.

@flash-gordon
Copy link
Member

and ofc it'll require to run more queries to system tables/views to get the info about composite types

@solnic
Copy link
Member Author

solnic commented Jan 30, 2017

@flash-gordon oh that's interesting, thanks!

@jodosha jodosha removed this from the v1.0.0.beta1 milestone Feb 6, 2017
@jodosha jodosha self-assigned this Feb 6, 2017
@jodosha
Copy link
Member

jodosha commented Feb 6, 2017

I'm closing this in favor of #377, which includes @solnic's work too. Thank you @solnic

@jodosha jodosha closed this Feb 6, 2017
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