Add Plug to Transform Parameters #149

jherdman · 2018-12-24T17:26:22Z

This, optional, Plug can be used to transform incoming dasherized
parameters to underscored ones that are more amenable to usage in your
application.

Resolves #145

test/jsonapi/plugs/transform_parameters_test.exs

lib/jsonapi/plugs/transform_parameters.ex

lib/jsonapi/utils/underscore.ex

jherdman · 2018-12-24T20:52:06Z

TODO

Add documentation to README
Resolve all feedback

snewcomer

Nice work and could/will clean up a lot of custom plugs/transformations ppl may have used! So essentially we have incoming transformation and outgoing transformation. I think it might be good to make these obviously clear in the README. Overall a user has to understand, to underscore params, add a plug. To underscore a response object, add a config option.

lib/jsonapi/utils/underscore.ex

lib/jsonapi/plugs/transform_parameters.ex

lib/jsonapi/utils/underscore.ex

test/jsonapi/plugs/transform_parameters_test.exs

lib/jsonapi/plugs/transform_parameters.ex

lib/jsonapi/plugs/underscore_parameters.ex

jherdman · 2018-12-27T14:59:44Z

Overall a user has to understand, to underscore params, add a plug. To underscore a response object, add a config option...

This asymmetry kind of stinks. Maybe we could optionally include the plug with the config option set?

jherdman · 2018-12-27T15:27:34Z

OK, current set of TODOs I'll try to wrap up over the next while:

Automatically dasherize incoming params via the config option?
Resolve duplication in transforming params in the QueryParser plug
Resolve Add Plug to Transform Parameters #149 (comment) — should we do this here, or wait for the other PR?

jherdman · 2018-12-27T15:28:47Z

mix.exs

+          "README.md",
+          "LICENSE.md"
+        ],
+        main: "readme"


I hope this is okay. There are so many goodies in this file, so we may as well include it.

snewcomer · 2018-12-28T00:30:15Z

test/jsonapi/plugs/content_type_negotiation_test.exs

      )
      |> ContentTypeNegotiation.call([])

    assert conn.halted

    assert Plug.Conn.get_resp_header(conn, "content-type") == [
-             "application/vnd.api+json; charset=utf-8"
+             "#{mime_type()}; charset=utf-8"


nice cleanup 👍

snewcomer · 2018-12-28T00:32:56Z

README.md

+
+## Dasherizing Fields
+
+JSONAPI now recommends the use of dashes (`-`) in place of underscore (`_`) as a


note this is now camelCase But that will happen in a follow up PR.

snewcomer · 2018-12-28T00:35:07Z

lib/jsonapi/utils/underscore.ex

+  end
+  def dash(value) do
+    value
+  end


following up on the previous conversation, I think the swap of method names could come in another PR or in #137. This looks great as is!

snewcomer · 2018-12-28T00:35:33Z

README.md

+JSONAPI now recommends the use of dashes (`-`) in place of underscore (`_`) as a
+word separator. Enabling this requires two quick steps:
+
+1. Dasherizing *outgoing* fields requires you to set the `underscore_to_dash`


README.md

jherdman · 2018-12-28T15:33:55Z

I'm pretty sure this is in a "done" state now.

I'd like to recommend that a breaking 1.0 release project be opened as a result of this PR. Maybe we should sneak in a few other breaking changes whilst we're at it?

snewcomer · 2018-12-28T16:12:15Z

@jherdman Nice work! Sry for asking, but what is the breaking change here? This just adds functionality, right? I most likely missed something.

Also, I think there are a few items we could think about for 1.0.

profile links https://jsonapi.org/format/1.1/. (~January 2019) albeit non breaking
camelCase member names (if we decide to switch over to). This is totally up in the air and we could stick with conventions in the Elixir community

doomspork · 2018-12-28T16:21:31Z

README.md

@@ -19,7 +19,7 @@ A project that will render your data models into [JSONAPI Documents](http://json

 ## Documentation

-[Full docs here](https://hexdocs.pm/jsonapi/api-reference.html)
+[Full docs here](https://hexdocs.pm/jsonapi)


Should we include a link to the actual JSONAPI spec here?

jherdman · 2018-12-28T16:28:13Z

In this PR just that one MUST use this new plug to get underscored attrs. This means QueryParser doesn't do this on it's own (I can revert this if you'd like). On master: the deprecations module was renamed (we should also mark this as private if it isn't); we moved one plug, and I think renamed it. On Fri., Dec. 28, 2018, 11:12 Scott Newcomer <notifications@github.com wrote:

@jherdman <https://github.com/jherdman> Nice work! Sry for asking, but what is the breaking change here? This just adds functionality, right? I most likely missed something. Also, I think there are a few items we *could* think about for 1.0. 1. profile links https://jsonapi.org/format/1.1/. (~January 2019) albeit non breaking 2. camelCase member names (if we decide to switch over to). This is totally up in the air and we could stick with conventions in the Elixir community — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#149 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAM5PEMsJHGX-zwbSI_m5Df-dI_w2J5ks5u9kLfgaJpZM4Zgl0P> .

On Dec. 28, 2018 11:12, "Scott Newcomer" <notifications@github.com> wrote: @jherdman <https://github.com/jherdman> Nice work! Sry for asking, but what is the breaking change here? This just adds functionality, right? I most likely missed something. Also, I think there are a few items we *could* think about for 1.0. 1. profile links https://jsonapi.org/format/1.1/. (~January 2019) albeit non breaking 2. camelCase member names (if we decide to switch over to). This is totally up in the air and we could stick with conventions in the Elixir community — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#149 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAM5PEMsJHGX-zwbSI_m5Df-dI_w2J5ks5u9kLfgaJpZM4Zgl0P> .

jherdman · 2018-12-28T16:29:02Z

Totally down with that. I'll do so ASAP. It'd probably be prudent to note which versions of the spec are supported.

…

On Fri., Dec. 28, 2018, 11:21 Sean Callan ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In README.md <#149 (comment)>: > @@ -19,7 +19,7 @@ A project that will render your data models into [JSONAPI Documents](http://json ## Documentation -[Full docs here](https://hexdocs.pm/jsonapi/api-reference.html) +[Full docs here](https://hexdocs.pm/jsonapi) Should we include a link to the actual JSONAPI spec here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#149 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAM5JleT4t6P0cZ7G7s_iR4ry9zBiHaks5u9kUMgaJpZM4Zgl0P> .

jherdman · 2018-12-28T17:08:07Z

lib/jsonapi/plugs/query_parser.ex

@@ -179,7 +186,7 @@ defmodule JSONAPI.QueryParser do
      if inc =~ ~r/\w+\.\w+/ do
        acc ++ handle_nested_include(inc, valid_includes, config)
      else
-        inc = inc |> dash() |> String.to_existing_atom()
+        inc = String.to_existing_atom(inc)


@snewcomer here's an example of a backwards incompat change. I could add it back in with a deprecation warning.

Since we are doing the whole shebang, probably ok.

Heh... I should have checked the PR first. I can go back to the hard deprecation if you'd prefer.

README.md

snewcomer · 2018-12-30T01:06:17Z

lib/jsonapi/plugs/query_parser.ex

@@ -179,7 +186,7 @@ defmodule JSONAPI.QueryParser do
      if inc =~ ~r/\w+\.\w+/ do
        acc ++ handle_nested_include(inc, valid_includes, config)
      else
-        inc = inc |> dash() |> String.to_existing_atom()
+        inc = String.to_existing_atom(inc)


Since we are doing the whole shebang, probably ok.

snewcomer · 2018-12-30T01:16:52Z

README.md

@@ -80,6 +84,30 @@ when Ecto gets more complex field selection support we will go further to only q

 You will need to handle filtering yourself, the filter is just a map with key=value.

+## Dasherized Fields
+
+JSONAPI now recommends the use of dashes (`-`) in place of underscore (`_`) as a


I'd be curious as to @doomspork or @jeregrine opinion what the library case default should be and if we should take into consideration camelCase recommendation now. Or should we stick with general Elixir conventions for underscore and leave as it, providing configuration on top to convert to dash or camelCase based on what the consumer wants?

As someone using this lib I'd prefer to opt into whatever I want, even on a request level. Imagine an API like Stripe's where you tell the API what version of it you're expecting. In such a case I'd want to either dasherize for old clients, or camelize for modern ones. FWIW as a default I'd love to see at least dashes given that this is the v1.0 recommendation.

jherdman · 2018-12-30T01:19:23Z

Reverted the hard deprecation on QueryParser dasherizing. This wasn't a lot of fun when I tried this branch out in my project.

jherdman · 2018-12-30T01:36:07Z

Initial testing has show that this also underscore values, e.g. if I POST /sources with the payload like this:

{
  "data": {
    "attributes": {
      "math-problem": "1-1=2"

I'll get:

{
  "data": {
    "attributes": {
      "math-problem": "1_1=2"

jherdman · 2018-12-30T15:18:59Z

I've beefed up the tests to account for the above problems, and am successfully using this branch in a project. I'm reasonably sure this is good to go unless there's any more feedback.

doomspork · 2018-12-30T16:04:23Z

@jherdman can you please undo the change LICENSE → LICENSE.md, we do not need to add a .md extension. There's really no reason to make these changes as part of this PR. Please please keep PRs focused on the work you're doing and avoid unnecessary changes 👍

jherdman · 2018-12-30T18:57:07Z

The reason for this change is to include it in the docs. ExDoc gets cranky otherwise. Apologies about this PR ballooning.

…

On Sun., Dec. 30, 2018, 11:04 Sean Callan ***@***.*** wrote: @jherdman <https://github.com/jherdman> can you please undo the change LICENSE → LICENSE.md, we do not need to add a .md extension. There's really no reason to make these changes as part of this PR. Please *please* keep PRs focused on the work you're doing and avoid unnecessary changes 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#149 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAM5Arg5eVQPI-4BwTJKrclQuucIwIxks5u-OQHgaJpZM4Zgl0P> .

This, optional, Plug can be used to transform incoming dasherized parameters to underscored ones that are more amenable to usage in your application. Resolves beam-community#145

Includes docs for the new `UnderscoreParamters` plug, and tweaks a few other things to make the narrative slightly better.

jherdman · 2018-12-31T01:07:16Z

@doomspork reverted!

jherdman · 2019-01-04T14:29:19Z

@doomspork anything else I can do? I'd love to put this PR to bed :)

doomspork · 2019-01-04T18:00:41Z

@snewcomer any final thoughts before I merge this? I believe we will need @jeregrine to cut a release once we have everything merged and ready.

snewcomer

Really nice work @jherdman.

snewcomer · 2019-01-04T17:15:48Z

lib/jsonapi/deprecation.ex

+
+  def warn(:query_parser_dash) do
+    IO.warn(
+      "`JSONAPI.QueryParser` will no longer automatically dasherize incoming parameters. Please include `JSONAPI.UnderscoreParameters` in your pipeline. See https://github.com/jeregrine/jsonapi/pull/149",


Should we add ...dasherize incoming parameters as of version 1.0.0?

Let's do that in a separate PR so @jherdman can get this off his plate.

doomspork requested changes Dec 24, 2018

View reviewed changes

test/jsonapi/plugs/transform_parameters_test.exs Outdated Show resolved Hide resolved

lib/jsonapi/plugs/transform_parameters.ex Outdated Show resolved Hide resolved

lib/jsonapi/utils/underscore.ex Outdated Show resolved Hide resolved

snewcomer reviewed Dec 24, 2018

View reviewed changes

lib/jsonapi/utils/underscore.ex Outdated Show resolved Hide resolved

lib/jsonapi/plugs/transform_parameters.ex Outdated Show resolved Hide resolved

lib/jsonapi/plugs/transform_parameters.ex Outdated Show resolved Hide resolved

jherdman force-pushed the transform-incoming-parameters branch 2 times, most recently from 0deefb1 to 28af691 Compare December 26, 2018 16:08

jherdman commented Dec 26, 2018

View reviewed changes

lib/jsonapi/utils/underscore.ex Outdated Show resolved Hide resolved

jherdman force-pushed the transform-incoming-parameters branch from 28af691 to 13fb4d2 Compare December 26, 2018 16:32

doomspork requested changes Dec 26, 2018

View reviewed changes

test/jsonapi/plugs/transform_parameters_test.exs Outdated Show resolved Hide resolved

lib/jsonapi/plugs/transform_parameters.ex Outdated Show resolved Hide resolved

jherdman force-pushed the transform-incoming-parameters branch from 13fb4d2 to e5c57a3 Compare December 27, 2018 03:27

jherdman commented Dec 27, 2018

View reviewed changes

lib/jsonapi/plugs/underscore_parameters.ex Outdated Show resolved Hide resolved

jherdman commented Dec 27, 2018

View reviewed changes

jherdman force-pushed the transform-incoming-parameters branch from b2a9bf7 to a4b62a6 Compare December 27, 2018 15:40

snewcomer reviewed Dec 28, 2018

View reviewed changes

jherdman force-pushed the transform-incoming-parameters branch 2 times, most recently from 55a12ce to 74e2a39 Compare December 28, 2018 15:33

doomspork reviewed Dec 28, 2018

View reviewed changes

jherdman force-pushed the transform-incoming-parameters branch from 74e2a39 to 4b09c4a Compare December 28, 2018 17:06

jherdman commented Dec 28, 2018

View reviewed changes

snewcomer reviewed Dec 30, 2018

View reviewed changes

jherdman force-pushed the transform-incoming-parameters branch from 4b09c4a to 6aa4d6f Compare December 30, 2018 01:18

jherdman force-pushed the transform-incoming-parameters branch 2 times, most recently from f603e42 to d89b1a4 Compare December 30, 2018 01:32

jherdman force-pushed the transform-incoming-parameters branch 3 times, most recently from 18be57a to 89d8176 Compare December 30, 2018 15:07

doomspork requested review from jeregrine and snewcomer December 30, 2018 16:06

jherdman added 4 commits December 30, 2018 20:05

Add Plug to Transform Parameters

85b74c2

This, optional, Plug can be used to transform incoming dasherized parameters to underscored ones that are more amenable to usage in your application. Resolves beam-community#145

Constantize JSON:API Mime Type

3837b6a

Spruce Up Docs

6b9e717

Includes docs for the new `UnderscoreParamters` plug, and tweaks a few other things to make the narrative slightly better.

Soft Deprecate JSONAPI.QueryParser.dash/1

5d3110e

jherdman force-pushed the transform-incoming-parameters branch from 89d8176 to 5d3110e Compare December 31, 2018 01:05

doomspork approved these changes Jan 4, 2019

View reviewed changes

snewcomer approved these changes Jan 4, 2019

View reviewed changes

doomspork merged commit bc11890 into beam-community:master Jan 4, 2019

jherdman mentioned this pull request Jan 7, 2019

[RFC] Tweak Utils.Underscore Module #153

Closed

3 tasks

jherdman deleted the transform-incoming-parameters branch January 8, 2019 13:45

jherdman mentioned this pull request Jan 12, 2019

Release Planning #157

Closed

5 tasks

snewcomer mentioned this pull request Jan 16, 2019

Provide camelized option according to v1.1 spec #158

Merged

jherdman mentioned this pull request Jan 17, 2019

Expand fields to simplify string functions snewcomer/jsonapi#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Plug to Transform Parameters #149

Add Plug to Transform Parameters #149

jherdman commented Dec 24, 2018

jherdman commented Dec 24, 2018

snewcomer left a comment

jherdman commented Dec 27, 2018

jherdman commented Dec 27, 2018 •

edited

Loading

jherdman Dec 27, 2018

snewcomer Dec 28, 2018

snewcomer Dec 28, 2018

snewcomer Dec 28, 2018

snewcomer Dec 28, 2018

jherdman commented Dec 28, 2018

snewcomer commented Dec 28, 2018

doomspork Dec 28, 2018

jherdman Dec 28, 2018

jherdman commented Dec 28, 2018 via email

jherdman commented Dec 28, 2018 via email

jherdman Dec 28, 2018

snewcomer Dec 30, 2018

jherdman Dec 30, 2018

snewcomer Dec 30, 2018

snewcomer Dec 30, 2018

jherdman Dec 30, 2018

jherdman commented Dec 30, 2018

jherdman commented Dec 30, 2018

jherdman commented Dec 30, 2018

doomspork commented Dec 30, 2018

jherdman commented Dec 30, 2018 via email

jherdman commented Dec 31, 2018

jherdman commented Jan 4, 2019

doomspork commented Jan 4, 2019

snewcomer left a comment

snewcomer Jan 4, 2019

doomspork Jan 4, 2019


		## Dasherizing Fields

		JSONAPI now recommends the use of dashes (`-`) in place of underscore (`_`) as a

Add Plug to Transform Parameters #149

Add Plug to Transform Parameters #149

Conversation

jherdman commented Dec 24, 2018

jherdman commented Dec 24, 2018

TODO

snewcomer left a comment

Choose a reason for hiding this comment

jherdman commented Dec 27, 2018

jherdman commented Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jherdman commented Dec 28, 2018

snewcomer commented Dec 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jherdman commented Dec 28, 2018 via email

jherdman commented Dec 28, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jherdman commented Dec 30, 2018

jherdman commented Dec 30, 2018

jherdman commented Dec 30, 2018

doomspork commented Dec 30, 2018

jherdman commented Dec 30, 2018 via email

jherdman commented Dec 31, 2018

jherdman commented Jan 4, 2019

doomspork commented Jan 4, 2019

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jherdman commented Dec 27, 2018 •

edited

Loading