chimney icon indicating copy to clipboard operation
chimney copied to clipboard

Add support for case conversions

Open ghostbuster91 opened this issue 3 years ago • 4 comments

Hi,

I though that I could use that feature in my current project.

For now I only took the changes from #162 made by @enhan and migrated it to the current codebase.

Once you confirm that this is still a valid approach I will write more tests, as discussed in the original PR.

ghostbuster91 avatar Oct 26 '20 08:10 ghostbuster91

Codecov Report

Merging #193 into master will decrease coverage by 0.30%. The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
- Coverage   99.51%   99.21%   -0.31%     
==========================================
  Files          21       22       +1     
  Lines         616      634      +18     
  Branches       66       71       +5     
==========================================
+ Hits          613      629      +16     
- Misses          3        5       +2     
Impacted Files Coverage Δ
...main/scala/io/scalaland/chimney/dsl/FlagsDsl.scala 88.88% <50.00%> (-11.12%) :arrow_down:
...o/scalaland/chimney/internal/NameTransformer.scala 100.00% <100.00%> (ø)
...laland/chimney/internal/macros/MappingMacros.scala 100.00% <100.00%> (ø)
...ney/internal/macros/TransformerConfigSupport.scala 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1bafc74...f62a7ce. Read the comment docs.

codecov[bot] avatar Oct 26 '20 08:10 codecov[bot]

Hi, thanks for taking care of progressing that feature 👍

The only issue I see is that when you define two independent flags for naming conversion, one may enable them both and it would be cumbersome to understand how the library behaves.

I'd suggest to parameterize the flag with the conversion kind so that user is able to:

  • disableNamingConversion
  • enableNamingConversion(SnakeToCamel)
  • enableNamingConversion(CamelToSnake)

krzemin avatar Oct 26 '20 18:10 krzemin

Hi,

The only issue I see is that when you define two independent flags for naming conversion, one may enable them both and it would be cumbersome to understand how the library behaves.

I was just trying to stick with what was discussed earlier (https://github.com/scalalandio/chimney/pull/162#pullrequestreview-407799283), but I see your point. Although for me it isn't about understanding, but rather about an artificial use-case this case covers. I cannot image why somebody would have both snake case and camel case naming conventions within a single class.

I'd suggest to parameterize the flag with the conversion kind so that user is able to:

They wouldn't remain flags, would they? Such parametricity is achieved in chimney through TransformerCfg. If you want them to be mutually exclusive then I think that there is no other option. Or am I missing sth?

Although we could disable the rest of namingConversions when enabling one. Is this what you had in mind?

ghostbuster91 avatar Nov 23 '20 10:11 ghostbuster91

This idea didn't allow to enable both conversion, but rather pick source and target encoding. It left some space for representing invalid conversions (like: from A to A), but it was easily fixable by some implicit requirement on type difference.

The current proposal allows to do x.enableSnakeToCamel.enableCamelToSnake (and disable them separately) which brings a lot of questions about library behavior (one conversion would be applied? both? none of them?). It's better to eliminate such ambiguities by better API design.

I cannot image why somebody would have both snake case and camel case naming conventions within a single class.

That question would be valid if we allow to specify conversion per field. But I guess we don't want to go that far.

They wouldn't remain flags, would they?

It depends how you define a flag :) So far we had only boolean (2-state) flags. With this proposal, we would need to extend this definition and introduce 3-state flag (conversion disabled, enabled SnakeToCamel, enabled CamelToSnake). I'm fine with this.

And yes, when we do x.enableNamingConversion(SnakeToCamel).enableNamingConversion(CamelToSnake), we only keep the latest setting. And we disable the whole name conversion feature with disableNamingConversion, not each flag separately which is a bit cleaner IMO.

Other advantage of having this setting as a flag is that we can use it in scoped (implicit) configuration for free as long as the operation is defined in FlagsDsl. If you'd like to keep it in TransformerCfg, you'd probably need to extend io.scalaland.chimney.dsl.TransformerConfiguration in order to properly support setting naming conversion in scoped configuration.

krzemin avatar Nov 23 '20 19:11 krzemin

Closing due to inactivity.

krzemin avatar Feb 24 '23 20:02 krzemin