Remove top level links if configured#88
Conversation
lib/jsonapi/serializer.ex
Outdated
| data: encoded_data, | ||
| included: flatten_included(to_include) | ||
| } | ||
| |> merge_links(data, view, conn, Application.get_env(:jsonapi, :remove_links, false)) |
There was a problem hiding this comment.
Could we refactor this to avoid piping into a single function?
snewcomer
force-pushed
the
remove_links
branch
from
2d37352 to
9e2ab26
Compare
lib/jsonapi/serializer.ex
Outdated
| data: encoded_data, | ||
| included: flatten_included(to_include) | ||
| } | ||
| merge_links(encoded_data, data, view, conn, Application.get_env(:jsonapi, :remove_links, false)) |
There was a problem hiding this comment.
Since we're using Application.get_env(:jsonapi, :remove_links, false) in a number of places, would you be open to refactoring this for us?
I think we can create a function:
defp remove_links?, do: Application.get_env(:jsonapi, :remove_links, false)|
|
||
| refute relationships[:links] | ||
| refute encoded[:data][:links] | ||
| refute encoded[:links] |
There was a problem hiding this comment.
Do we need to add tests to ensure it's not removed elsewhere?
|
Thank you @snewcomer 😁 @jeregrine any thoughts? |
|
Just pinging this again. Thanks for the reviews! |
doomspork
left a comment
doomspork
left a comment
There was a problem hiding this comment.
@snewcomer apologizes for the delays on getting additional feedback. I found two small things, if you could fix those I think we're safe to merge.
lib/jsonapi/serializer.ex
Outdated
| data: encode_rel_data(rel_view, rel_data) | ||
| } | ||
| |> merge_related_links(info, Application.get_env(:jsonapi, :remove_links, false)) | ||
| |> merge_related_links(info, remove_links?) |
lib/jsonapi/serializer.ex
Outdated
| } | ||
|
|
||
| doc = merge_links(encoded_data, data, view, conn, Application.get_env(:jsonapi, :remove_links, false)) | ||
| doc = merge_links(encoded_data, data, view, conn, remove_links?) |
There was a problem hiding this comment.
Could you please update calls to remove_links? to remove_links?()
|
Thank you @snewcomer 👍 Until @jeregrine can cut a new release you'll need to use master: {:jsonapi, github: "jeregrine/jsonapi"} |
|
Sorry for delay on this. I hope to jump back in and cut a new release next
week. Thanks for covering Sean!
…On Tue, Dec 19, 2017 at 5:29 PM, Sean Callan ***@***.***> wrote:
Merged #88 <#88>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#88 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGKFqLA9mHq_srDStsucAk82WEY4AIsks5tCEbegaJpZM4RA8-j>
.
|
|
You got it @jeregrine! |
3 participants
Looks like links can be both top level and resource. This removes the top level as well if configured.