hamiltop / rethinkdb_ecto

Shim library for using Ecto with RethinkDB. Not a full adapter.
24 stars 4 forks source link

Initial Feedback #1

Open AdamBrodzinski opened 8 years ago

AdamBrodzinski commented 8 years ago

Firstly.... you rock man! When I get back to the bay the beers are on me! :beers:

I added a resource on my Phoenix app using this and overall it was very seamless... any issues I ran into were from me not understanding how things in Ecto worked (like the schema types and changesets).

I've never used Ecto outside of a few tutorials but the whole thing seemed seamless... I just had to find/replace Repo with MyConnection and everything just worked. I didn't use any callbacks (not sure if that affects this package) but i'm sure i'll run into a use case soon for them.

I took some notes of issues that I ran into:

It doesn't have the get!, delete! and update! bang methods so you end up checking for nil in the controller and then responding appropriately there. Phoenix by default has these in their scaffold so that if get! throws it returns a JSON error (I think that's what happens at least i'll have to double check). Not sure if adding bang methods is something that should be added?

# controller scaffold example
  def delete(conn, %{"id" => id}) do
    user = Repo.get!(User, id)

    # Here we use delete! (with a bang) because we expect
    # it to always work (and if it does not, it will raise).
    Repo.delete!(user)

    send_resp(conn, :no_content, "")
  end

And also there wasn't a clear way to select a default database so that threw an error (["Tabletest.remindersdoes not exist."]). This isn't really an Ecto adapter issue though.

Does the Rethink driver have this somewhere? I wasn't sure what the name: :foo opts were in the docs so I tried those but this threw an error on the first request: worker(Badger.DB, [name: :badger]])

[info] Sent 500 in 37ms
[error] #PID<0.360.0> running Badger.Endpoint terminated
Server: localhost:4000 (http)
Request: GET /v2/reminders
** (exit) exited in: :gen_server.call(Badger.DB, :conn_opts)
    ** (EXIT) no process

For testing this I just used the test db and all was well.

Overall this is phenomenal.... it really makes it easy to use Phoenix! Please let me know if theres anything I can help test. I'm going to try and finish up the API migration in the next couple of weeks.

hamiltop commented 8 years ago

http://hexdocs.pm/rethinkdb/RethinkDB.Connection.html#start_link/1 tells you how to specify default db in start_link

I skipped the bang methods just for the proof of concept. Definitely will add them in.

Callbacks should all be supported. There's probably a few bugs in my implementation, but the callback functions defintely should get called.

Instead of the find/replace, you could just gut Repo and have it call use RethinkDB.Ecto.Connection and everything would theoretically work. Then Repo.get would work out of the box.

name: foo should be better documented, but that's a standard GenServer opt for registering the process with a name. When you do use RethinkDB.Connection (or in this case RethinkDB.Ecto.Connection) it automatically adds name: __MODULE__ and uses __MODULE__ as the named process whenever run is called.

I'm going to throw this up on the ecto mailing list for a bit of feedback, but your initial feedback is promising.

AdamBrodzinski commented 8 years ago

Oh snap, I forgot that Hex gives you docs... I was sifting through source code :laughing: Thanks!

Right good idea, using use in Repo should do the trick!

Ah, yea gotta read up on the GenServer stuff... i've kind of just put that on the back burner until I can get comfy with Elixir :smile: