Add a withOverrides method to ConfigFieldMapping #259
Conversation
|
Not much to comment, nice and useful addition. Are you planning to add a README part about this somewhere in another PR or you think it's not worth it? I would like to have this somewhere in the README. |
|
I'll add this to the README later, it is surely useful to have it there. |
There was a problem hiding this comment.
This is a very nice addition. Thanks, @ruippeixotog.
I made a small suggestion. Take it or leave it.
| * uses this mapping. | ||
| */ | ||
| def withExceptions(exceptions: (String, String)*) = { | ||
| val map = exceptions.toMap |
There was a problem hiding this comment.
A slightly terser take:
ConfigFieldMapping(exceptions.toMap.withDefault(apply))
There was a problem hiding this comment.
Nicely done, I didn't think of using withDefault here :) Changed the method to your suggestion in 213551c.
# Conflicts: # CHANGELOG.md
| /** | ||
| * Returns a `ConfigFieldMapping` that uses this mapping with some exceptions. | ||
| * | ||
| * @param exceptions the exceptions for this mapping as pairs (field, configKey) |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This may seem nitpicky, but I wonder about the naming. Exceptions makes me think this will throw Exceptions. Maybe something like name overrides?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@ruippeixotog I'd probably go with withOverrides.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@melrief, what do you think about this? We need a fifth opinion here :)
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
All right, renamed to withOverrides in 274b3de! :)
ruippeixotog
changed the title
Closes #256 and #204.