Ability to specify name of resource id #22
Conversation
Defaults to using "id".
| conn.params["id"] | ||
| resource_id -> | ||
| conn.params[resource_id] | ||
| end |
There was a problem hiding this comment.
This is a really minor thing, but I think this block could be moved into a small function e.g.
id = get_resource_id(opts)
defp get_resource_id(opts) do
case opts[:id] do
nil ->
conn.params["id"]
resource_id ->
conn.params[resource_id]
end
end
There was a problem hiding this comment.
Great stuff! Love it, see pushed commits.
|
Just a few small comments! 😃 |
Embrace the functional paradigm.
Also fix a typo or two.
| @@ -103,6 +103,17 @@ defmodule PlugTest do | |||
| assert load_resource(conn, opts) == expected | |||
| end | |||
|
|
|||
| test "it loads the resource correctly with opts[:id] specified" do | |||
| opts = [model: Post, id: "user_id"] | |||
There was a problem hiding this comment.
Do you wanna call it id_name instead of just id to go with resource_name and not confuse with the actual id?
There was a problem hiding this comment.
I was just thinking that actually! 👍
To go with resource_name and not confuse with the actual id.
| @@ -222,26 +225,37 @@ defmodule Canary.Plugs do | |||
| defp fetch_resource(conn, opts) do | |||
| repo = Application.get_env(:canary, :repo) | |||
|
|
|||
| id_name = get_resource_id_name(conn, opts) | |||
There was a problem hiding this comment.
I'm sorry to keep nitpicking on this, but while the opts key should be id_name, I think the variable on this line should just be id, and the function should be get_resource_id rather than get_resource_id_name, since we are really just getting the id.
That's my fault: I should have been more clear in my last comment. Other than that, I think this looks good!
So, the opts key is id_name, but this variable on line 228 should just be id, and the function should be get_resource_id.
Let me know if you disagree! 😸
There was a problem hiding this comment.
Sorry mate, I was too quick searching and replacing, I fully agree seeing as the variable on 228 isn't the name of the id parameter but the actual id 🙈
Let me fix it real quick.
Use id for the id and id_name for the name of the id parameter..
|
✌️ I see you added Travis integration! Great! |
Add ability to specify name of resource id
|
Beauty! Merged 👌 |
| @@ -103,6 +103,17 @@ defmodule PlugTest do | |||
| assert load_resource(conn, opts) == expected | |||
| end | |||
|
|
|||
| test "it loads the resource correctly with opts[:id_name] specified" do | |||
| opts = [model: Post, id_name: "user_id"] | |||
There was a problem hiding this comment.
This should probably have been post_id though
Instead of assuming resource id is named "id" this PR let's you override it and specify the name of the resource id while defaulting to "id" if none is specified.
Use it like so: