ficus icon indicating copy to clipboard operation
ficus copied to clipboard

hyphen-case config key support

Open frohoff opened this issue 10 years ago • 8 comments
trafficstars

This is a draft PR to implement #7 using Guava's CaseFormat class for the camelCase to hyphen-case conversion.

Created separate HyphenCaseArbitraryTypeReader trait and object (existing ArbitraryTypeReader API/behavior is unchanged) that invokes a new macro that specifies a hyphen-case NameMapper trait implementation that gets passed down to the extractMethodArgsFromConfig() method. Added a test case to ArbitraryTypeReaderSpec.

Probably could be reorganized a bit. Please advise.

frohoff avatar May 06 '15 01:05 frohoff

Thanks, @frohoff! This is a useful addition. I really like the idea of passing a name transformation into extractMethodArgsFromConfig, and I think we should go forward with this.

Here are a couple thoughts, though.

Currently Ficus's only runtime dependency is Typesafe config, and I'd prefer to keep it that way. I've gotten into a dependency nightmare with Guava version mismatches before, and they are no fun. CaseFormat is indeed handy, but I'm not sure that we should be deciding whether people want to use hyphens, underscores, etc.

Separately, it doesn't seem to me that the NameMapper trait is providing any more benefit than a simple nameMapper: String => String could provide.

Therefore, I propose that we do the following:

  • Get rid of NameMapper, and instead use a String => String argument (which can be identity for the existing code)
  • Get rid of the Guava dependency and let users pass in their own function, which may be CaseFormat.LOWER_CAMEL.to(CaseFormat.LOWER_HYPHEN, _) or whatever other transformation they care to make. They can then create their own HyphenCaseArbitraryTypeReader (or equivalent) trait/object.

What do you think?

ceedubs avatar May 06 '15 15:05 ceedubs

I had actually originally intended to implement with a user-specifiable mapping function it similar to how you suggested but it was seeming quite complicated to pass parameters through the macro definition into the implementation and helper methods, though my experience with macros is pretty limited and it's possible there's something I'm not considering. With this in mind, I was hesitant to do anything too convoluted in the implementation, and without a mechanism to pass parameters through, it would probably leave ficus library users to write their own macro definitions which seemed like something one would want to avoid.

I'm definitely open to suggestions/direction.

frohoff avatar May 06 '15 21:05 frohoff

Ah, thanks for the explanation. I didn't realize it was so difficult to pass custom arguments to macros. I'm wondering if we could still use String => String without NameMapper though it looks like we will still have to do it via macro like you have said.

I'm still pretty hesitant to introduce the Guava dependency. Maybe it would make sense to have a separate module that could add the Guava dependency and have helpers for a few different formats? I don't know, I'll ponder this a bit.

ceedubs avatar May 07 '15 12:05 ceedubs

Agreed on dropping NameMapper in favor of String => String. In the meantime I'll see if I can get something working using annotations. Also, we can consider making Guava an optional (compile-time only) dependency that people need to include on their own and just ensure that all load-time dependencies to it are in a separate class file so that it doesn't throw a NoClassDefFoundError when using the existing ArbiraryTypeReader code without Guava on the classpath.

frohoff avatar May 07 '15 14:05 frohoff

I would say don't sink too much time using the annotation approach for now. It's complicated and probably less flexible than other approaches. I have some ideas I would like to try out, but it might be a few days before I get to it.

ceedubs avatar May 07 '15 15:05 ceedubs

One nice thing about the annotation approach is that it might enable further customization of these readers for things like the exhaustive key-parameter mapping I described in #13.

frohoff avatar May 07 '15 16:05 frohoff

I threw together a rough working prototype that supports customizable parameter mapping and exhaustivity checking (for #13) using annotations at frohoff/ficus@54cb7d12d09faabca05350cdfce23a9ff4253967 but then had another idea:

Another way to do this that seemed a bit cleaner was to have the macro compile in calls/references to members on the trait/object it's being called "through" (the macro's c.prefix) that could be overridden for customization by users:

A rough working prototype for this is at frohoff/ficus@57de54d8b369b49912dc983c402d52694936a395

This ends up looking roughly like this:

// user code
object CustomizedArbitraryTypeReader extends ArbitraryTypeReader {
  override val checkExhaustivity: Boolean = true
  override def mapParams(n: String) = CaseFormat.LOWER_CAMEL.to(CaseFormat.LOWER_HYPHEN, n)
}

// library code
trait ArbitraryTypeReader {
  implicit def arbitraryTypeValueReader[T]: ValueReader[T] = macro ArbitraryTypeReaderMacros.arbitraryTypeValueReader[T]
  val checkExhaustivity: Boolean = false
  def mapParams(n: String) = n
}

This should also allow implementing common options as modular traits a la:

// user code
object MyArbitraryTypeReader 
  extends ArbitraryTypeReader 
  with GuavaHyphenParamMapping // optional guava dependency or in separate module
  with ExhaustivityChecking

frohoff avatar May 13 '15 18:05 frohoff

Looking forward for this to be merged.

shanielh avatar Nov 26 '15 11:11 shanielh