-
Notifications
You must be signed in to change notification settings - Fork 12
feat: insert expect #318
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
base: master
Are you sure you want to change the base?
feat: insert expect #318
Conversation
ahoy-jon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some review
| test("Dataframe should validate expect all with __ in it") { | ||
| for { | ||
| people <- Dataset(Person("Louis", 50), Person("Lara", 50)) | ||
| result <- people.toDF.expectAll(row(__, 50)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to write?
for {
people <- Dataset(Person("Louis", 50), Person("Lara", 50))
result <- people.expectAll(row(__, 50))
| object row { | ||
| def apply[T](predicate: T => Boolean): ConditionalMatcher[T] = ConditionalMatcher(predicate) | ||
| def apply[T](value: T): DataMatcher[T] = DataMatcher(value) | ||
| def apply(value: Any, values: Any*): RowMatcher = RowMatcher(Row(value +: values: _*)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def apply(valueMatcher: ValueMatcher, valueMatcher*: ValueMatcher*):RowMatcherwith different ways to build a valueMatcher.
implicit def conversion[T:ToValueMatcher](t:T):ValueMatcher
implicit def keyConversion[T:ToValueMatcher](t2:(String,T)):ValueMatcher
with possible future syntaxes being
row(__, "name" -> ((_:String).isEmpty || isNull))
row(__, "adr[0]" -> row[Adr](_.countryCode == "FR)
row(__, "name" -> (v:String) => v == null || v.isEmtpy) // could reuse at lowcost zio-test macros on assertTrue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about datasets ? You only tackle dataframes with this syntax no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we take :
row(__, "name" -> ((_:String).isEmpty || isNull), 20)
There are both positional and key based predicates, what it means ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should work on dataset too.
we could have
expectAll(
row(_.isAdult).allowMultipleMatches,
row("toto", 13))There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in Scala, it doesn't work. We could make it work, but that is too ambiguous.
def default[T]: T => Boolean = _ => true
def checkMatch( age: Int => Boolean = default,
surname: String=> Boolean = default,
name: String => Boolean =default):Any = ???
def __ = ???
checkMatch(__, name = _.contains("toto"))
checkMatch(__, name = _.contains("toto"), default)| Dataset(1, 2, 3).flatMap(_.expectAll(row(1), row(2))) | ||
| } @@ failing, | ||
| test("Dataset should validate expect all with conditional match") { | ||
| Dataset(1, 2, 3).flatMap(_.expectAll(row(_ > 0))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only if a matcher is set to match more rows (allowMultipleMatches)
|
@ahoy-jon I would like a review on the current state of this PR. I think we can improve error handling, but I don't know exactly how. I would also need more insight about blocking keys based on the current state. |

summarize,
validity:
summarize :
Due to the potential large volume of data, and low volume of rows, the idea is to keep certain rows to explain the failing test :
The purge works per rules, keeping at least N values in the two sets, depending on closeness