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

Error validation transformer (VTransformer) #120

Closed
wants to merge 5 commits into from

Conversation

ulanzetz
Copy link
Contributor

@ulanzetz ulanzetz commented Oct 29, 2019

This PR adds chimney-validated module with VTransformer, which can use unsafe transformations like Option unwrapping (can be usefull with proto3 generated case classes (#118)), int from string parsing etc, but unlike enableUnsafeOption all errors are aggregated to chain and contains full path to failed fields. In my code I use cats.data.Validated, but it can be rewritten by own error-chaining semigroup implementations.

Also. now it's limited by 22 fields in single case class.

I'd be thankful for review and feedback

@ulanzetz ulanzetz force-pushed the vtransformer branch 2 times, most recently from 87eb9d1 to 58c1ca0 Compare October 29, 2019 11:12
@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #120 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  Coverage   95.99%   95.99%           
=======================================
  Files           8        8           
  Lines         449      449           
  Branches       64       64           
=======================================
  Hits          431      431           
  Misses         18       18
Impacted Files Coverage Δ
...scalaland/chimney/internal/TransformerMacros.scala 95.3% <100%> (ø) ⬆️

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 d69b3a4...8a66fd8. Read the comment docs.

@ulanzetz ulanzetz force-pushed the vtransformer branch 6 times, most recently from dc88af4 to 8a3f159 Compare October 30, 2019 15:28
@ulanzetz ulanzetz force-pushed the vtransformer branch 2 times, most recently from b786441 to 5b93ccb Compare November 6, 2019 12:19
@sideeffffect
Copy link

can I somehow help with this? I would really like to use it 😃

@sideeffffect
Copy link

you probably need to update scoverage to 1.6.1+
scoverage/sbt-scoverage#295

@REDNBLACK
Copy link

@krzemin Can we (me and @ulanzetz) ask u kindly, to give a feedback and tell what need to be done in order for this PR to get approved? Or do u have anything against increased complexity/etc?

@sideeffffect
Copy link

sideeffffect commented Dec 11, 2019

@REDNBLACK @ulanzetz the Travis CI build is failing for Scala 2.13. Upgrading scoverage might help, as I've hinted at scoverage/sbt-scoverage#295
Having a green CI may be a prerequisite for getting feedback 🤷‍♂️

@ulanzetz
Copy link
Contributor Author

@sideeffffect scoverage version is already 1.6.1. Master build also failed on 2.13

@sideeffffect
Copy link

maybe try changing the Travis config, so that we test on 2.13.1 (currently it's 2.13.0)

@ulanzetz
Copy link
Contributor Author

Thanks, @sideeffffect. Bumping version to 2.13.1 in travis config helped

@ulanzetz
Copy link
Contributor Author

@krzemin, what about this PR? Could you give any feedback for this. Does it make sense to continue?

@krzemin
Copy link
Member

krzemin commented Jan 22, 2020

Hi @ulanzetz, I was finally able to take a look at your PR. Below some general thoughts.

  1. I think that it would be very much desired to have some form of Transformation returning Validated/Either/... #118 implemented in Chimney.
  2. What is most problematic in the proposal, is that the whole macro is duplicated in order to support validation transformers. That would make maintenance of library much more difficult as any change or bugfix would have to be applied in two independent places. It's necessary to have derivation backbone implemented once that works for both cases and extract only crucial differences.
  3. Direct dependency on Cats is troublesome in terms of being dependent on cats release cycle. It also seems wrong from the architectural standpoint: any integration with specific, external library should happen in minimal pluggable module, but the whole core implementation should not rely on any specific validation wrapper type.
  4. If 2. and 3. can be addressed somehow, I would like to see such validated transformers as part of core library (along with total transformers and patchers), hence creating separate module/artifact would not be necessary.
  5. Ideally other data types than cats' Validated (like Option, Try, any F[_] for which we can provide glueing code) should be supported.

Thank you for your contribution effort - I think that it totally make sense to continue working on this. I can offer some hints how to tackle these issues.

@ulanzetz
Copy link
Contributor Author

Resolved as part of #145

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.

4 participants