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

hyphen-case config key support #13

Closed
wants to merge 1 commit into from

Conversation

shanielh
Copy link

Just a rebase of ceedubs#12 to iheartradio/ficus

@kailuowang
Copy link
Member

@shanielh thanks for the rebase. I had the same thoughts as @ceedubs (the guava dependency). Also we have the plan to rewrite the automatic case class readers based on shapeless. I am working on some shapeless+cat facilities that will make even more easier. Hopefully we'll see that soon. So for now I am going to leave this one on hold for a bit. Please check back later.

@@ -5,6 +5,9 @@ description := "A Scala-friendly wrapper companion for Typesafe config"

startYear := Some(2013)

licenses := Seq(
"MIT License" -> url("http://www.opensource.org/licenses/mit-license.html")
)
Copy link
Member

Choose a reason for hiding this comment

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

Not needed here, license is moved to project/Publish.scala

@ceedubs
Copy link

ceedubs commented Jan 14, 2016

Travis Brown has done some recent work to support different cases in Circe. Since it uses Shapeless for derivation, it might be a good reference.

@kailuowang
Copy link
Member

@ceedubs, that looks quite similar, thanks for the hint.

@larroy
Copy link

larroy commented Feb 28, 2016

Please don't add huge dependencies such as Guava.

@colindean
Copy link

Hyphen case would be super useful for my team. What work needs to be done to get this into Ficus?

@kailuowang
Copy link
Member

@colindean the plan was to take advantage of henkan to revamp the currently macro based auto case class mapping. However henkan is having a weird bug kailuowang/henkan#15. I would like to take advantage of @milessabin's SI2712 fix to simplify henkan before jumping into that bug fix though...
so it might take a while.

@colindean
Copy link

Ah. I guess we'll stick to snake case for now!

@kailuowang
Copy link
Member

Okay. I gave up on using henkans here. So for this PR we just need to replace CaseFormat.LOWER_CAMEL.to(CaseFormat.LOWER_HYPHEN, name
with a hand-roll implementation and get rid of the guava dependency, then we can merge.

@shanielh
Copy link
Author

@kailuowang Great! I can do the changes, But before I do them, Maybe I should expose the NameMapper trait and get it in the arbitrary type reader as an implicit parameter? That way everyone can define their own NameMapper. In that case we can get rid of the guava dependency and even not implement an HypenCaseNameMapper.

@joprice
Copy link

joprice commented May 12, 2016

@shanielh that's the exact conversation that Kai and I just had about this design. I think that's cleaner and more extensible.

@shanielh
Copy link
Author

@joprice Hah, Ok, I believe I'll fix the PR tomorrow 👍

@kailuowang
Copy link
Member

@shanielh thanks very much!

@shanielh
Copy link
Author

Continued in #22 👍

@shanielh shanielh closed this May 13, 2016
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.

None yet

7 participants