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

ArbitraryTypeReader.NameMapper #22

Merged
merged 1 commit into from
May 13, 2016

Conversation

shanielh
Copy link

A new way to define name conventions to use in ficus

Continue to #13

* @param nameMapper The name mapper from the implicit scope, or the default name mapper if not found
* @return The name mapper to be used in current implicit scope
*/
def get(implicit nameMapper: NameMapper = DefaultNameMapper) = nameMapper

Choose a reason for hiding this comment

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

hmm, not sure how I feel about the name get here, usually we made such implicit instance summon helper method the apply method.

Copy link
Author

Choose a reason for hiding this comment

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

Will change it :)

@kailuowang
Copy link

LGTM, @joprice wanna take a look as well?

@@ -22,12 +59,13 @@ class ArbitraryTypeReaderMacros(val c: blackbox.Context) extends ReflectionUtils
new ValueReader[T] {
def read(config: Config, path: String): T = instantiateFromConfig[T](
config = c.Expr[Config](Ident(TermName("config"))),
path = c.Expr[String](Ident(TermName("path")))).splice
path = c.Expr[String](Ident(TermName("path"))),
mapper = c.Expr[NameMapper](q"""net.ceedubs.ficus.readers.NameMapper()""")).splice
Copy link

Choose a reason for hiding this comment

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

I think you want to add _root_ here to preserve hygiene (since someone could use this inside a package which contains a subpackage named net. See the notes on hygiene here http://docs.scala-lang.org/overviews/quasiquotes/hygiene.

A new way to define name conventions to use in ficus
@shanielh
Copy link
Author

@kailuowang @joprice Fixed all the comments, instead the one in the tests, Where I tried to follow the convention of the rest of the tests.

@kailuowang
Copy link

Thanks very much @shanielh I'll merge after build pass and @joprice has plan to release it soon after.

@kailuowang kailuowang merged commit 8702a10 into iheartradio:master May 13, 2016
@shanielh shanielh deleted the feature/name-mapper branch May 13, 2016 16:37
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.

3 participants