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

Fields filters should be underscored #193

Open
zlatio opened this issue Nov 14, 2016 · 9 comments
Open

Fields filters should be underscored #193

zlatio opened this issue Nov 14, 2016 · 9 comments

Comments

@zlatio
Copy link

zlatio commented Nov 14, 2016

Hello,

First of all thank you for this library :) I think it's great. However I have one issue. When passing fields parameter like fields[users]=first-name,last-name I get argument_error

** (ArgumentError) argument error
     stacktrace:
       :erlang.binary_to_existing_atom("first-name", :utf8)
       (elixir) lib/string.ex:1792: String.to_existing_atom/1
       (elixir) lib/enum.ex:1184: Enum."-map/2-lists^map/1-0-"/2
       lib/ja_serializer/builder/attribute.ex:29: JaSerializer.Builder.Attribute.do_filter/2

Attributes filters needs to be underscored because otherwise they can't be converted to existing atoms. So in the attribute builder on line 31 we need something like this:

  defp safe_atom_list(field_str) do
    field_str |> String.replace("-", "_") |> String.split(",") |> Enum.map(&String.to_existing_atom/1)
  end

What do you think?

@zlatio zlatio changed the title Fields should be underscored Fields filters should be underscored Nov 14, 2016
@yordis
Copy link
Contributor

yordis commented Jan 29, 2017

@alanpeabody there is no default naming as far as I know http://jsonapi.org/format/#document-member-names so I would like to see by default use underscore instead of dashes.

Specially when most of the programming languages are easier to read from underscore and probably most of the people that use this come from a background where underscores are normally the default name. I barely see dashes been use.

Could we default the attributes name to underscore please 🙏 . It's normal for me to go and add

config :ja_serializer,
    key_format: :underscored

in my configuration in every single project that use this

@alanpeabody
Copy link
Contributor

@yordis the default naming is what jsonapi.org recommends: http://jsonapi.org/recommendations/#naming

@yordis
Copy link
Contributor

yordis commented Jan 29, 2017

😭 I guess I will continue using the code snippet

@alanpeabody
Copy link
Contributor

sorry. :-/

@zlatio
Copy link
Author

zlatio commented Jan 30, 2017

@alanpeabody @yordis I am not sure you got the exact issue I have with this. As per JSONAPI spec

Member names SHOULD contain only the characters “a-z” (U+0061 to U+007A), “0-9” (U+0030 to U+0039), and the hyphen minus (U+002D HYPHEN-MINUS, “-“) as separator between multiple words.

Thing is when I pass multiple words separated by - like fields[users]=first-name,last-name I get

** (ArgumentError) argument error
     stacktrace:
       :erlang.binary_to_existing_atom("first-name", :utf8)
       (elixir) lib/string.ex:1792: String.to_existing_atom/1
       (elixir) lib/enum.ex:1184: Enum."-map/2-lists^map/1-0-"/2
       lib/ja_serializer/builder/attribute.ex:29: JaSerializer.Builder.Attribute.do_filter/2

because actually there is no first-name but first_name and it can't be converted to existing atom. Right now I am using a plug to convert those but I see it easy to patch like:

  defp safe_atom_list(field_str) do
    field_str |> String.replace("-", "_") |> String.split(",") |> Enum.map(&String.to_existing_atom/1)
  end

I am just not sure if this is the best solution :)

@alanpeabody
Copy link
Contributor

@zlatio yeah, I understand your issue. To be frank I don't know the best solution. Currently the formatting of the keys all happens here:

https://github.com/vt-elixir/ja_serializer/blob/master/lib/ja_serializer/param_parser.ex

However it is impractical/incorrect to format values there. The piece your solution is missing is a way to customize it... If we hooked into Application.get_env(:ja_serializer, :key_format) and did it conditionally I would be happy to accept a PR.

@zlatio
Copy link
Author

zlatio commented Jan 30, 2017

True, there is no way to customize it. I will try to come up with a PR for this :)

@alanpeabody
Copy link
Contributor

Thanks a bunch!

@habeel
Copy link

habeel commented Mar 29, 2017

I think the handling is inconsistent through out if by default inserted_at is being serialized to "inserted-at" then the generated templates should also handle the input the same way.

I believe ParamParser should be called inside Params.to_attributes because Params.to_attributes is taking care of parsing relationships which is a json-api spec so then it should also take care of translating the keys.

I will open up a new issue for this.

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

No branches or pull requests

5 participants