W-11222217: generate error message when attempting to migrate mel emp… #685
Conversation
|
|
||
| it should "fail with empty literal" in { | ||
| Migrator.migrate("payload == empty").metadata.children.head shouldBe NonMigratable("expressions.emptyLiteral") | ||
| } |
There was a problem hiding this comment.
Should we migrate to isEmpty(payload)
There was a problem hiding this comment.
I looked into it, but the empty comparison differs from isEmpty in a couple of cases:
0 == emptyandisEmpty(0)false == emptyandisEmpty(false)
Maybe the error message should be a bit more specific?
There was a problem hiding this comment.
I see now we could migrate it to payload == 0 or payload == false or isEmpty(payload)
There was a problem hiding this comment.
Or migrate it to just isEmpty(payload) with a warning saying that some use cases may not work. But I prefer to migrate it to something that just throw an error. I could imagine not many people using those weird semantics of the == empty
There was a problem hiding this comment.
I see now we could migrate it to
payload == 0 or payload == false or isEmpty(payload)
I can add this, but there is one other case that I forgot to add, empty also returns true when compared against a string with only whitespaces. That is a case that we can't recognize because we would need to know if the argument is a String to trim it in the comparison.
I also don't imagine that a lot of people use this feature, but it feels wrong to migrate it with only a warning if we are potentially breaking the application. If you think it's not a problem I can migrate it to this case and add in the message that it breaks if the argument was a string with only whitespaces.
There was a problem hiding this comment.
So it would be payload == 0 or payload == false or isEmpty(payload) or (payload is String and isBlank(payload)) The problem for me is that we have two options one is that we try to migrate with the most elegant but less compatible or we can try to migrate with the more compatible but less readable (elegant) that is this big expression. I'm ok with both things either migrate to isEmpty(payload) with a warning saying this may not work in all cases or migrate to ugly expression. @svacas what do you prefer?
There was a problem hiding this comment.
I would go with the nice expression + warning, that should cover most of the cases and go along with the MMA philosophy of doing a best effort without getting too convoluted
There was a problem hiding this comment.
Nice expression being just isEmpty(payload) right?
| @@ -195,4 +195,8 @@ class MigratorTest extends FlatSpec with Matchers { | |||
| it should "migrate mel expression with isCausedBy function" in { | |||
| Migrator.migrate("exception.causedBy(org.mule.RuntimeException)").getGeneratedCode() shouldBe "%dw 2.0\n---\nJava::isCausedBy(error.cause, 'org.mule.RuntimeException', false)" | |||
| } | |||
|
|
|||
| it should "fail with empty literal" in { | |||
There was a problem hiding this comment.
description should be something like migrate mel expression with empty and raise warning
if possible verify the migrated expression and also using != besides ==
…ty literal