Fix for issue #92 #93
Conversation
| @@ -51,8 +51,7 @@ internal fun TypeUniverse.convertToKTypeUniverse(): KTypeUniverse { | |||
| } | |||
|
|
|||
| private class KTypeDomainConverter( | |||
| private val typeDomain: TypeDomain, | |||
| private val isTransform: Boolean | |||
There was a problem hiding this comment.
isTransform was unused, decided to nix it now since it was convenient.
| @@ -62,7 +61,7 @@ private class KTypeDomainConverter( | |||
|
|
|||
| typeDomain.types.forEach { | |||
| when(it) { | |||
| DataType.Int, DataType.Symbol, DataType.Ion -> { /* intentionally blank */ } | |||
| DataType.Int, DataType.Symbol, DataType.Ion, DataType.Bool -> { /* intentionally blank */ } | |||
There was a problem hiding this comment.
Also a minor addition that stopped my IDE from complaining about missing a case.
| (sum sum_a | ||
| (who) | ||
| (cares a::product_to_remove) | ||
| (noti (a product_to_remove))) |
There was a problem hiding this comment.
noti is a record while cases is a product.
Same thing the other two are: they are just nonsense names.
There was a problem hiding this comment.
Ok, the name can still be kind of confusing. Is there any meaning to the name noti?
| @@ -7318,10 +7318,12 @@ class DomainA private constructor() { | |||
| */ | |||
| fun productA( | |||
| one: Long, | |||
| two: ProductToRemove, | |||
There was a problem hiding this comment.
Unrelated to this change, but the formatting of permuted domain visitor transforms is slightly off:
open fun transformUnpermutedProduct(node: DomainA.UnpermutedProduct): DomainB.UnpermutedProduct {
val new_foo = transformUnpermutedProduct_foo(node)
val new_bar = transformUnpermutedProduct_bar(node)
val new_metas = transformUnpermutedProduct_metas(node)
return DomainB.UnpermutedProduct(
foo = new_foo,
bar = new_bar,
metas = new_metas
)
}
There was a problem hiding this comment.
That's been there for a while. It's also not that important, IMHO, to get generated code to be perfectly formatted.
| @@ -129,6 +129,7 @@ | |||
| (product product_to_remove whatever::symbol) | |||
| (record record_to_remove (irrelevant int)) | |||
| (sum sum_to_remove (doesnt) (matter)) | |||
| (sum will_be_replaced_with_product (will_be_replaced_with_product_variant t::product_to_remove)) | |||
There was a problem hiding this comment.
Not too big a fan of this type's name. Could it be renamed to start with product to be more consistent with the preceding types? Something like product_to_be_replaced? Also could remove the "will_be_replaced_with_product_" part before "variant".
Consider adding a comment saying this will be replaced in the permuted domain with another product of the same name.
There was a problem hiding this comment.
It's a sum type that will be replaced with a product.
| (record record_a (one int) (two product_to_remove)) | ||
| (product product_a | ||
| one::int | ||
| // This reference to a typed removed in the permuted domain must not cause pig to crash: |
There was a problem hiding this comment.
(non-blocking) nit: "This reference to a type removed in the..."
Similarly for L150 and L156
| (sum sum_a | ||
| (who) | ||
| (cares a::product_to_remove) | ||
| (noti (a product_to_remove))) |
There was a problem hiding this comment.
Ok, the name can still be kind of confusing. Is there any meaning to the name noti?
Fix for issue #92.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.