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

Supports snakecase. #268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jkugiya
Copy link

@jkugiya jkugiya commented Jul 22, 2018

Close #170

@morpheyesh
Copy link

Hope this will be merged soon!

Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

Thanks, @jkugiya. The feature looks good to me. I wonder if we shouldn't use the simple implementation based on regexs.

trait SnakeCaseJsonSupport extends DefaultJsonProtocol {

override protected def extractFieldNames(classTag: ClassTag[_]): Array[String] = {
def snakify(name: String) = {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should also just use the regex here? Performance shouldn't be an issue as the formats are usually only evaluated once per type.

val sampleFields2 = SampleFields2().toJson.asJsObject
// Then
val fields1 = sampleFields1.fields
fields1.contains(snakify("HelloWorld")) mustEqual true
Copy link
Member

Choose a reason for hiding this comment

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

Good that snakify is a different implementation here in the test, but I would prefer if the test case expectations would be spelled out.

val sampleFields2 = SampleFields2().toJson.asJsObject
// Then
val fields1 = sampleFields1.fields
fields1.contains(snakify("HelloWorld")) mustEqual true
Copy link
Member

Choose a reason for hiding this comment

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

Good that snakify is a different implementation here in the test, but I would prefer if the test case expectations would be spelled out.

Copy link
Member

Choose a reason for hiding this comment

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

E.g. like this:

Suggested change
fields1.contains(snakify("HelloWorld")) mustEqual true
fields1 must haveKey("hello_world")

@plokhotnyuk
Copy link

@jrudolph Regexs are so slow...

More simple and efficient functions which can enforce kebab case too are here

@jkugiya
Copy link
Author

jkugiya commented Apr 22, 2019

@jrudolph Regexs are so slow...

Yes, the reason for this implementation is regex's performance. But I can understand regex is easier to maintain. I created another PR(#295). I'll leave it up for either.

@jkugiya
Copy link
Author

jkugiya commented Apr 22, 2019

@plokhotnyuk I also tried your implementation. But some of the behavior is unexpected for me in some corner case.

ex)

  • ^There-arE_Non_(Alphabetic))_character!S -> ^_there_-ar_e__non_(_alphabetic_))_character_!s (The capital letter is followed by _.)
  • -> There are space character -> _there_ are_ space_ character_ (it start with _)

It is difficult to judge which is better because there aren't standard specification for this conversion, as far as I know.

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.

Please support snake case
4 participants