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

Provide camelized option according to v1.1 spec #158

Merged
merged 10 commits into from
Jan 20, 2019

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Jan 15, 2019

https://jsonapi.org/format/1.1/

Profile links to follow in another PR.

@snewcomer snewcomer self-assigned this Jan 15, 2019
camelCase fields:

```
config :jsonapi, field_transformation: :camelize
Copy link
Contributor

Choose a reason for hiding this comment

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

Ought this to be the default going forward, or do you think that explicit configuration is more ideal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of goes back to this discussion but I agree it probably should be the default going forward. Other libraries like ja_serializer also assume the default according to the jsonapi spec. I'll wait for @doomspork's thoughts as well.

#149 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

If this is the JSONAPI default, we should follow suit 👌

def camelize(value) when is_binary(value) do
case Regex.split(~r{([a-zA-Z0-9])(?<delimeter>[-_])([a-zA-Z0-9])}, to_string(value), on: [:delimeter]) do
words ->
[h | t] = words |> Enum.filter(&(&1 != ""))
Copy link
Member

Choose a reason for hiding this comment

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

Can we please find another way to express this block of code? The use of pipelines and lists is making this very difficult to grok. It's also strange that we have a case but don't actually pattern match on things.

Also, do we need Enum.filter(&(&1 != "")? What example would leave you with an extra empty string? If this is something that occurs I think trim: true in Regex.split/2 would be a better option.

Copy link
Member

@doomspork doomspork Jan 16, 2019

Choose a reason for hiding this comment

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

defmodule Example do
  @camelize_regex ~r/([a-zA-Z0-9])(?<delimeter>[-_])([a-zA-Z0-9])/
  def camelize(value) when is_binary(value) do
    with [h | t] when length(t) > 0 <- Regex.split(@camelize_regex, value, on: [:delimeter], trim: true),
         capitalized <- Enum.map(t, &String.capitalize/1),
         downcased <- String.downcase(h)
    do
      Enum.join([downcased | capitalized])
    else
      _ -> value
    end
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

I updated my example above to use the length(t) > 0 guard because I found an issue with the original code (and mine) using this regex:

Utils.String.camelize("exampleName")
"examplename"

I do not believe we want to downcase strings that are already camelcased.

Copy link
Member

Choose a reason for hiding this comment

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

We can also update our regex here to avoid the need for the on: [:delimeter] (delimiter is spelled wrong in the regex also):

~r/(?<=[a-zA-Z0-9])[-_](?=[a-zA-Z0-9])/

iex> Regex.split(~r/(?<=[a-zA-Z0-9])[-_](?=[a-zA-Z0-9])/, "example_name")
["example", "name"]

Copy link
Member

Choose a reason for hiding this comment

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

@snewcomer / @jherdman I feel this file as gotten to be a bit complicated with lots of functions for each style of formatting. What do you think about simplifying this module a bit?

defmodule Example do
  def underscore(value) do
    value
    |> tokenize()
    |> join("_")
  end

  def dasherize(value) do
    value
    |> tokenize()
    |> join("-")
  end

  def camelize(value) do
    with [h | t] when length(t) > 0 <- tokenize(value),
         capitalized <- Enum.map(t, &String.capitalize/1),
         downcased <- String.downcase(h)
    do
      Enum.join([downcased | capitalized])
    else
      _ -> value
    end
  end

  defp join(value, joiner), do: Enum.join(value, joiner)

  defp tokenize(value) do
    replaced_value = String.replace(value, ~r/([a-z])(?=[A-Z])/, "\\1_\\2")
    tokens = Regex.split(~r/(?<=[a-zA-Z0-9])[-_](?=[a-zA-Z0-9])/, replaced_value, trim: true)
    Enum.map(tokens, &String.downcase/1)
  end
end
iex> Example.underscore("example_name")
"example_name"
iex> Example.underscore("example-name")
"example_name"
iex> Example.underscore("exampleName")
"example_name"

iex> Example.dasherize("example-name")
"example-name"
iex> Example.dasherize("example_name")
"example-name"
iex> Example.dasherize("exampleName")
"example-name"

iex> Example.camelize("example_name")
"exampleName"
iex> Example.camelize("example-name")
"exampleName"
iex> Example.camelize("exampleName")
"exampleName"

Happy to open a PR to clean up this util 👍

camelCase fields:

```
config :jsonapi, field_transformation: :camelize
Copy link
Member

Choose a reason for hiding this comment

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

If this is the JSONAPI default, we should follow suit 👌

Enum.into(value, %{}, &camelize/1)
end

def camelize({key, value}) do
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere we make use of guards yet with this function we go the if/else route. Is there a reason we didn't use guards here as well?

Suggested change
def camelize({key, value}) do
def camelize({key, value}) when is_map(value) do
{camelize(key), camelize(value)}
end
def camelize({key, value})
{camelize(key), value}
end
end

@snewcomer
Copy link
Contributor Author

@doomspork @jherdman Lmk what you think about this PR to merge into this base branch. Essentially introduces expand_fields/2 where the second arg is the appropriate string method. There still exists Sean's idea to tokenize the string - which I like a lot and can happen here or another PR.

https://github.com/snewcomer/jsonapi/blob/5519920245c80af336027a9dfaafc526ec7cb8f7/lib/jsonapi/utils/string.ex

https://github.com/snewcomer/jsonapi/pull/1/files

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

## JSONAPI Support

This library implements [version 1.0](https://jsonapi.org/format/1.0/)
This library implements [version 1.1](https://jsonapi.org/format/1.1/)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this is strictly true, so it may be a bit misleading.

I'll start open a project board that has issues for new JSON:API v1.1 features.

@snewcomer snewcomer mentioned this pull request Jan 17, 2019
5 tasks
@doomspork
Copy link
Member

@snewcomer after we merge do you want me to open a PR to refactor the String utils?

@snewcomer
Copy link
Contributor Author

@doomspork Yeah that would work!

Just looking for some feedback on this PR before tying this one up. Basically consolidates duplicated logic to get at the value to transform.

@doomspork
Copy link
Member

Oh I missed that PR, let me look

@snewcomer snewcomer changed the title WIP - Provide camelized option according to v1.1 spec Provide camelized option according to v1.1 spec Jan 17, 2019
@snewcomer
Copy link
Contributor Author

@jherdman Did you want 0.9 released before this PR goes in?

@jherdman
Copy link
Contributor

Yeah, let's do that. I can't seem to find any issues that need to be addressed before a 0.9 release.

@snewcomer
Copy link
Contributor Author

Alrighty up to date with master and passing tests! All good to merge?

@doomspork doomspork merged commit 201ae1c into beam-community:master Jan 20, 2019
@snewcomer snewcomer deleted the sn/camel_case branch January 20, 2019 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants