almightycouch / rethinkdb_ecto

RethinkDB adapter for Ecto.
https://hexdocs.pm/rethinkdb_ecto
MIT License
114 stars 18 forks source link

Goyox86/improve options handling #18

Closed goyox86 closed 8 years ago

goyox86 commented 8 years ago

Hi People!

I'm starting a project and we are planning to use RethinkDB in a Phoenix app. I don't know if this is even the right way to do it but here we go.

I setup a small phoenix app and configured RethinkDB.Ecto and generated a small User model. Then I just ran mix ecto.create and mix ecto.migrate and according the output everything was fine! . I was so happy because I was good to go then I proceeded to fire up a iex session to create a User record which I did. Only to realize that even though mix ecto.create did create the DB I had configured mix ecto.migrate did not used that DB to create the users table on that DB instead it used the test DB which I coincidentally happen to have setup.

After a bit of Googling I came to the idea of setting the db option which is the one that RethinkDB Elixir driver expects. And tried again and I've got this:

 mix ecto.create                                                                                                            [20:49:21]
** (RuntimeError) Expected type STRING but found NULL.
    lib/rethinkdb_ecto.ex:119: RethinkDB.Ecto.storage_up/1
    lib/mix/tasks/ecto.create.ex:34: anonymous fn/2 in Mix.Tasks.Ecto.Create.run/1
    (elixir) lib/enum.ex:651: Enum."-each/2-lists^foreach/1-0-"/2
    (elixir) lib/enum.ex:651: Enum.each/2
    (mix) lib/mix/task.ex:296: Mix.Task.run_task/3
    (mix) lib/mix/cli.ex:58: Mix.CLI.run_task/2
    (elixir) lib/code.ex:363: Code.require_file/2

I keep trying combinations of options until I found a way to make mix ecto.create and ecto.migrate to yield the results I was expecting.

1) Set the database option and run mix ecto.create. 2) Setting the option db and then run ecto.migrate.

Which for someone coming from the Rails world does not make much sense so I treated this as a bug 😉

What this patch does is to hook into the options parsing and put into db and host (which are the options the driver expects) the values of the more conventional Ecto options database and hostname if they are provided giving precedence to db and host and then normalizing the usage across this library the usage of db in order to avoid inconsistencies. I've also added a line to make sure the users table is there when running the tests because I spent some time reading them while trying to run them.

After this you should be able to just drop in the dependency in a project and then run mix ecto.create and mix ecto.migrate with any combination of db,host,database and hostname options.

Tests are green of course! And thanks for the Adapter it will save me a lot of time because I really want to use RethinkDB in our project!

I'm not a en Elixir developer so I don't know if the code is idiomatic but you get the idea 😉

almightycouch commented 8 years ago

I'm closing this as the issue should be fixed in the latest release.