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

Map encoder with Value class #567

Merged

Conversation

cchantep
Copy link
Collaborator

@cchantep cchantep commented Oct 5, 2021

Closes #566

@cchantep cchantep force-pushed the feature/map-valueclass-typedencoder branch from f370ae2 to d947a30 Compare October 6, 2021 11:22
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #567 (68a7c97) into master (7b08c31) will decrease coverage by 0.80%.
The diff coverage is 93.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
- Coverage   96.35%   95.54%   -0.81%     
==========================================
  Files          63       63              
  Lines        1071     1101      +30     
  Branches        9        8       -1     
==========================================
+ Hits         1032     1052      +20     
- Misses         39       49      +10     
Flag Coverage Δ
2.12.15 95.54% <93.63%> (-0.81%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ataset/src/main/scala/frameless/TypedEncoder.scala 97.58% <93.24%> (-2.42%) ⬇️
...taset/src/main/scala/frameless/RecordEncoder.scala 93.58% <93.93%> (-6.42%) ⬇️
...t/src/main/scala/frameless/functions/package.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 7b08c31...68a7c97. Read the comment docs.

@cchantep cchantep force-pushed the feature/map-valueclass-typedencoder branch from d947a30 to fe18ef2 Compare October 10, 2021 12:42
@cchantep cchantep changed the title WIP: Fix #566 - Map encoder with Value class Fix #566 - Map encoder with Value class Oct 10, 2021

/**
* @tparam F the value class
* @tparam G the single field of the value class
* @tparam H the single field of the value class (with guarantee it's not a `Unit` value)
* @tparam V the inner value type
*/
implicit def valueClass[F : IsValueClass, G <: ::[_, HNil], H <: ::[_, HNil], V]
implicit def valueClass[F : IsValueClass, G <: ::[_, HNil], H <: ::[_ <: FieldType[_ <: Symbol, _], HNil], K <: Symbol, V, KS <: ::[_ <: Symbol, HNil]]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also capture keys evidence so it can be used to wrap/unwrap in the map/array/collection encoder of Value class

Copy link
Member

Choose a reason for hiding this comment

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

@cchantep thanks for clarifying it.


implicit val javaBigDecimalEncoder: TypedEncoder[java.math.BigDecimal] = new TypedEncoder[java.math.BigDecimal] {
def nullable: Boolean = false
override def toString: String = "bigDecimalEncoder"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Useful when debuging

@@ -210,29 +226,39 @@ object TypedEncoder {
)
}

implicit def arrayEncoder[T: ClassTag](implicit encodeT: TypedEncoder[T]): TypedEncoder[Array[T]] =
implicit def arrayEncoder[T: ClassTag](
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactor to support array of Value class with the typeclass RecordFieldEncoder (rather than the lower TypedEncoder one)

}

import shapeless.{HList, LabelledGeneric}
import shapeless.test.illTyped

import org.scalatest.matchers.should.Matchers

case class UnitsOnly(a: Unit, b: Unit)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved down

@cchantep
Copy link
Collaborator Author

@pomadchin This can be reviewed

@pomadchin
Copy link
Member

@cchantep wow a solid PR; will take some time to look through that in the beginning of the work week.

@cchantep
Copy link
Collaborator Author

Is that ok for you @pomadchin ?

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

Checked it on my project and works nice, thanks for this solid and exciting improvement! This PR adds an additional tests coverage as well.


/**
* @tparam F the value class
* @tparam G the single field of the value class
* @tparam H the single field of the value class (with guarantee it's not a `Unit` value)
* @tparam V the inner value type
*/
implicit def valueClass[F : IsValueClass, G <: ::[_, HNil], H <: ::[_, HNil], V]
implicit def valueClass[F : IsValueClass, G <: ::[_, HNil], H <: ::[_ <: FieldType[_ <: Symbol, _], HNil], K <: Symbol, V, KS <: ::[_ <: Symbol, HNil]]
Copy link
Member

Choose a reason for hiding this comment

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

@cchantep thanks for clarifying it.

@@ -54,15 +55,16 @@ object TypedEncoder {
Invoke(path, "toString", jvmRepr)
}

implicit val booleanEncoder: TypedEncoder[Boolean] = new TypedEncoder[Boolean] {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by the formatting that happened here, makes it harder to read the diff

Comment on lines +65 to +66
i3: Keys.Aux[H, KS],
i4: Values.Aux[H, VS],
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these constrains here?

i3: Keys.Aux[H, KS],
i4: Values.Aux[H, VS]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Required to call RecordFieldEncoder.valueClass (l80)

@pomadchin
Copy link
Member

@cchantep sorry that it took so long! I'm also willing to have a look into #571 this week.

@cchantep cchantep merged commit f25a99c into typelevel:master Oct 15, 2021
@cchantep cchantep deleted the feature/map-valueclass-typedencoder branch October 15, 2021 09:36
@pomadchin pomadchin added the bug label Oct 18, 2021
@pomadchin pomadchin changed the title Fix #566 - Map encoder with Value class Map encoder with Value class Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map[X, BigDecimal] not properly encoded
2 participants