Add to view custom attribute close #89#92
Conversation
doomspork
left a comment
doomspork
left a comment
There was a problem hiding this comment.
@snewcomer this is a good first go, I've left some feedback and questions. I do think we should think about whether we want to be passing conn into the functions, that opens the door for misuse and confusing behavior.
Let's think about it a bit and give @jeregrine a chance to share his thoughts 👍
lib/jsonapi/view.ex
Outdated
| end) | ||
| end | ||
|
|
||
| defp merge(intermediate_map, field, value) do |
There was a problem hiding this comment.
What does merge/3 offer us over just using Map.put/3 directly?
| visible_fields(data) | ||
| visible_fields = fields() -- hidden() | ||
|
|
||
| Enum.reduce(visible_fields, %{}, fn field, intermediate_map -> |
There was a problem hiding this comment.
This was to ensure the return type was a Map, which is what Map.take was doing before.
lib/jsonapi/view.ex
Outdated
|
|
||
| Enum.reduce(visible_fields, %{}, fn field, intermediate_map -> | ||
| try do | ||
| merge(intermediate_map, field, apply(__MODULE__, field, [data, conn])) |
There was a problem hiding this comment.
Can we do something like this to DRY things up a bit?
value =
try do
apply(__MODULE__, field, [data, conn])
rescue
_e -> Map.get(data, field)
end
Map.put(intermediate_map, field, value)
snewcomer
left a comment
snewcomer
left a comment
There was a problem hiding this comment.
@doomspork Good calls. So the only reason to also pass conn would be to get at the current user or something like that. So if your API is returning a list of users, for every user you could ensure that only the current logged in user's email was sent back. Lmk your thoughts!
| visible_fields(data) | ||
| visible_fields = fields() -- hidden() | ||
|
|
||
| Enum.reduce(visible_fields, %{}, fn field, intermediate_map -> |
There was a problem hiding this comment.
This was to ensure the return type was a Map, which is what Map.take was doing before.
lib/jsonapi/view.ex
Outdated
|
|
||
| Enum.reduce(visible_fields, %{}, fn field, intermediate_map -> | ||
| try do | ||
| merge(intermediate_map, field, apply(__MODULE__, field, [data, conn])) |
|
Thank you @snewcomer! I want to review #87 and then we can cut a new release 😁 |
2 participants
Ref #89