Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply optional dash-to-underscore to include keys #102

Merged
merged 4 commits into from
May 1, 2018

Conversation

mvrkljan
Copy link
Contributor

When specifying an include string in request which contains dashed entries (e.g. article.guest-authors) and library is configured to convert dashes to underscore guest-authors should be transformed to guest_authors.

Currently, without this an error is thrown caused by converting string to an unknown atom.

@@ -0,0 +1,29 @@
defmodule JSONAPI.Utils.Dash do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just converts from dash to underscore, where Underscore coverts from underscore to dash. Wasn't sure if I should just add a dash function to the Underscore module or create a separate module. Let me know which approach you prefer and I'll update the PR if needed. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏓 @jeregrine

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets merge this into one module, if you feel better renaming it thats fine just make sure the tests pass. But since they both use the same configuration and both have functionally the same responsibility I think having one will be better.

@mvrkljan
Copy link
Contributor Author

@jeregrine updated, thanks!

@mvrkljan
Copy link
Contributor Author

@jeregrine Anything else needed for this one?

@jeregrine jeregrine merged commit 064d568 into beam-community:master May 1, 2018
@jeregrine
Copy link

Thank you for the work on this! Sorry for the delay ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants