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

Support for scope-default transformer configuration #190

Merged
merged 31 commits into from
Oct 16, 2020

Conversation

krzemin
Copy link
Member

@krzemin krzemin commented Oct 9, 2020

This PR addresses #176. It's result of several prototyping rounds made in cooperation with @d12frosted.

More or less it implements the plan sketched here: #176 (comment)

TODO:

  • more test cases, especially of newly added flags combinations
  • provide analogous mechanism for patchers let's have it as a separate issue (Implicitly scoped patcher configurations #191)
  • rethink naming and placement of newly added types
  • update docs

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #190 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
+ Coverage   99.34%   99.37%   +0.02%     
==========================================
  Files          20       21       +1     
  Lines         614      636      +22     
  Branches       62       61       -1     
==========================================
+ Hits          610      632      +22     
  Misses          4        4              
Impacted Files Coverage Δ
...main/scala/io/scalaland/chimney/TransformerF.scala 100.00% <ø> (ø)
...a/io/scalaland/chimney/internal/macros/Model.scala 100.00% <ø> (ø)
...laland/chimney/internal/macros/PatcherMacros.scala 96.42% <ø> (ø)
.../main/scala/io/scalaland/chimney/Transformer.scala 100.00% <100.00%> (ø)
...main/scala/io/scalaland/chimney/dsl/FlagsDsl.scala 100.00% <100.00%> (ø)
...alaland/chimney/dsl/TransformerConfiguration.scala 100.00% <100.00%> (ø)
.../scalaland/chimney/dsl/TransformerDefinition.scala 100.00% <100.00%> (ø)
...scalaland/chimney/dsl/TransformerFDefinition.scala 100.00% <100.00%> (ø)
...la/io/scalaland/chimney/dsl/TransformerFInto.scala 100.00% <100.00%> (ø)
...ala/io/scalaland/chimney/dsl/TransformerInto.scala 100.00% <100.00%> (ø)
... and 7 more

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 4fd8f0c...c1bf4f4. Read the comment docs.

Copy link

@d12frosted d12frosted left a comment

Choose a reason for hiding this comment

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

LGTM, next step would be to pass default flags from top to the very bottom 💯

Thank you for taking your time to try this idea.

@krzemin
Copy link
Member Author

krzemin commented Oct 10, 2020

next step would be to pass default flags from top to the very bottom

@d12frosted it's already supported, see these test cases: https://github.com/scalalandio/chimney/pull/190/files#diff-8bf852c2438394dc69a2bd644c2a815eR1017 Or do you mean something else?

@krzemin krzemin mentioned this pull request Oct 12, 2020
@d12frosted
Copy link

🤔 yeah, you are right. For some reason I was confused since I didn't see ability to pass configuration to Transformer.define. But looking at the tests - seems like it works 😸 thanks

@krzemin
Copy link
Member Author

krzemin commented Oct 14, 2020

For some reason I was confused since I didn't see ability to pass configuration to Transformer.define.

Good point, let me elaborate.

Initially I thought that capturing implicit flags on Transformer.define and encoding them in the Transformer(F)Defnition would be a simple and clean idea. But to capture types passed by implicit instance, we would need third type parameter (for flags) and just calling it as Transformer.define[From, To] would not be possible any more.

I could fix it by making .define a whitebox macro and look for implicit inside, but... there is a simpler solution:

  • build Transformer(F)Definition flags on top of TransformerFlags.Default
  • require scoped flags only in .buildTransformer
  • merge both flags stacks after capturing both types

It keeps almost the same semantics, but bring less troubles for the users.

@d12frosted
Copy link

@krzemin ah I see. This is something I didn't consider in my PRs. Nicely done!

@ulanzetz ulanzetz self-requested a review October 15, 2020 07:52
@krzemin krzemin changed the title Control over default configurations of transformers Support for scope-default transformer configuration Oct 16, 2020
@krzemin krzemin merged commit c3a749a into master Oct 16, 2020
@krzemin krzemin deleted the feature/control-default-flags branch October 16, 2020 20:01
@d12frosted
Copy link

This is wonderful! Thank you @krzemin 🎉

@krzemin
Copy link
Member Author

krzemin commented Oct 17, 2020

Thank you for your efforts too @d12frosted!

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