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

Add a withOverrides method to ConfigFieldMapping #259

Merged
merged 6 commits into from Jun 9, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
@@ -1,3 +1,9 @@
### 0.7.3 (unreleased)

- New features
- `ConfigFieldMapping` now has a `withExceptions` method that allows users to easily define exceptions to an existing
mapping.

### 0.7.2 (May 29, 2017)

- Bug fixes
Expand Down
21 changes: 19 additions & 2 deletions core/src/main/scala/pureconfig/ConfigFieldMapping.scala
@@ -1,10 +1,27 @@
package pureconfig

/**
* A mapping between case class fields and their respective keys in the config.
*/
trait ConfigFieldMapping extends (String => String) {

def apply(fieldName: String): String

/**
* Returns a `ConfigFieldMapping` that uses this mapping with some exceptions.
*
* @param exceptions the exceptions for this mapping as pairs (field, configKey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruippeixotog It might be worth adding a comment explaining that any override will be applied globally throughout the configuration, e.g. if a ("foo", "bar") exception is provided it will affect all fields named foo regardless of which object they're in and will affect all bar config keys regardless of the path leading up to the .bar part.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the distinction between ConfigFieldMapping and a ProductHint provides that information. A ConfigFieldMapping has no type parameter and has no effect as an implicit. On the other hand, a ProductHint has a type parameter restricting the hint to certain types (possibly one or all). Different product hints may be configured with different mappings, so "any override will be applied globally throughout the configuration" isn't entirely accurate. Do you still think it would be better to improve the documentation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This may seem nitpicky, but I wonder about the naming. Exceptions makes me think this will throw Exceptions. Maybe something like name overrides?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruippeixotog I agree that I was wrong when I said "globally throughout the configuration." I didn't realize the scope that the ConfigFieldMapping was applied within was a ProductHint. (I probably should, but I didn't.)

Probably nothing needs to be added. I'm skeptical I could inform without confusing.

Sorry.

Copy link
Member Author

Choose a reason for hiding this comment

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

@leifwickland I understand the confusion, in older pureconfig versions ConfigFieldMapping was the object to provide implicitly to configure product derivation :) Since ProductHints were added, ConfigFieldMapping was made only one of the multiple things you can configure via a ProductHint.

@derekmorr yeah, I wonder about that too. I don't think that the current name is perfect, but I'm not sure if withOverrides isn't less explicit/correct in the context of a mapping... I ended up using withExceptions. @leifwickland, @melrief, @jcazevedo, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruippeixotog I'd probably go with withOverrides.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer withExceptions. They're effectively exceptions to the general mapping rule. I think any confusion users might have that this will throw exceptions can be dismissed by looking at the method's documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@melrief, what do you think about this? We need a fifth opinion here :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer overrides. While I agree on the meaning of exception in this case, I also think the name can be confusing. I like override because it can be seen as "override the mapping behavior for the given fields"

Copy link
Member Author

Choose a reason for hiding this comment

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

All right, renamed to withOverrides in 274b3de! :)

* @return a `ConfigFieldMapping` that maps fields using `exceptions` if the field is present there and otherwise
* uses this mapping.
*/
def withExceptions(exceptions: (String, String)*) = {
val map = exceptions.toMap
Copy link
Collaborator

Choose a reason for hiding this comment

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

A slightly terser take:

ConfigFieldMapping(exceptions.toMap.withDefault(apply))

Copy link
Member Author

Choose a reason for hiding this comment

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

Nicely done, I didn't think of using withDefault here :) Changed the method to your suggestion in 213551c.

ConfigFieldMapping { field => map.getOrElse(field, this.apply(field)) }
}
}

object ConfigFieldMapping {

/**
* Creates a ConfigFieldMapping from the provided function, mapping names in
* the object that will receive config values to names in the configuration
Expand Down Expand Up @@ -32,9 +49,9 @@ object ConfigFieldMapping {
*/
def apply(typeFieldConvention: NamingConvention, configFieldConvention: NamingConvention): ConfigFieldMapping = {
if (typeFieldConvention == configFieldConvention) {
apply(identity(_))
apply(identity)
} else {
apply(typeFieldConvention.toTokens _ andThen configFieldConvention.fromTokens _)
apply(typeFieldConvention.toTokens _ andThen configFieldConvention.fromTokens)
}
}
}
28 changes: 28 additions & 0 deletions core/src/test/scala/pureconfig/ConfigFieldMappingSuite.scala
@@ -0,0 +1,28 @@
package pureconfig

import org.scalatest.{ FlatSpec, Matchers }

class ConfigFieldMappingSuite extends FlatSpec with Matchers {

behavior of "ConfigFieldMapping"

it should "allow defining a mapping using a function" in {
val mapping = ConfigFieldMapping(_.replace("Field", "ConfigKey"))
mapping("theBeautifulField") === "theBeautifulConfigKey"
mapping("theUglyFld") === "theUglyFld"
}

it should "allow defining a mapping between two naming conventions" in {
val mapping = ConfigFieldMapping(CamelCase, SnakeCase)
mapping("theBeautifulField") === "the_beautiful_field"
mapping("theUglyFld") === "the_ugly_fld"
}

it should "allow defining mappings with some exceptions" in {
val mapping = ConfigFieldMapping(CamelCase, SnakeCase).withExceptions(
"theUglyFld" -> "the_ugly_field")

mapping("theBeautifulField") === "the_beautiful_field"
mapping("theUglyFld") === "the_ugly_field"
}
}