add case for enum return types in multi-branch case statement #2766
Conversation
fulghum
left a comment
fulghum
left a comment
There was a problem hiding this comment.
Looks good for fixing the issue within a case statement. Just a couple suggestions around adding some comments to help future code readers understand why the code is special cased here.
| }, | ||
| }, | ||
| { | ||
| Skip: true, |
There was a problem hiding this comment.
I think it's generally useful to add some context if we're skipping a test. Could be what's missing to make the test pass or anything else that could be useful to someone looking at this brand new next year and trying to figure out why the test is skipped.
| }, | ||
| Assertions: []ScriptTestAssertion{ | ||
| { | ||
| Skip: true, |
There was a problem hiding this comment.
e.g. it might be helpful to add a comment linking to the GitHub issue for these skipped tests. That would help provide more context for a future code reader.
| if right == types.Null { | ||
| return left | ||
| } | ||
| if types.IsEnum(left) && types.IsEnum(right) { |
There was a problem hiding this comment.
Probably worth adding a comment to explain what we're doing here. This seems pretty difficult to reverse engineer and figure out in a year or two when someone new looks at this code and tries to figure out why these cases are in here.
Hotfix for Enum conversion issue:
dolthub/dolt#8598
Doesn't address real issue, which revolves around conversion for
CASE.