[v1] Remove UNKNOWN AST enum variant #1688
Conversation
| @@ -107,7 +102,7 @@ public static DatetimeField parse(@NotNull String value) { | |||
| case "SECOND": return SECOND(); | |||
| case "TIMEZONE_HOUR": return TIMEZONE_HOUR(); | |||
| case "TIMEZONE_MINUTE": return TIMEZONE_MINUTE(); | |||
| default: return UNKNOWN(); | |||
| default: throw new IllegalArgumentException("No enum constant DatetimeField." + value); | |||
There was a problem hiding this comment.
(self-review): behavior follows what Kotlin does for its valueOf() method, which throws an IllegalArgumentException if the string value does not match any of the enum variants.
| public static final int UNK = 3; | ||
| public static final int TRUE = 0; | ||
| public static final int FALSE = 1; | ||
| public static final int UNKNOWN = 2; |
There was a problem hiding this comment.
(self-review): change of UNK -> UNKNOWN is in reference to this recent PR comment -- #1679 (comment).
alancai98
force-pushed
the
remove-unknown-enum
branch
from
6c626b4 to
d810722
Compare
CROSS-ENGINE-REPORT ❌
Testing Details
Result Details
Now FAILING Tests ❌The following 5 test(s) were previously PASSING in BASE but are now FAILING in TARGET: Click here to see
Now IGNORED Tests ❌The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. Now Passing Tests180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-E2A5528). Before merging, confirm they are intended to pass. The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact. CROSS-COMMIT-REPORT ✅
Testing DetailsResult Details
|
| @@ -14,13 +14,8 @@ | |||
| */ | |||
| @EqualsAndHashCode(callSuper = false) | |||
| public class Scope extends AstEnum { | |||
There was a problem hiding this comment.
Where is Scope used in the AST? This might be a holdover from @t vs T which I think shouldn't be "scope" but more like some kind of qualifier boolean on the ExprVar/ExprId or whatever we are calling it.
There was a problem hiding this comment.
Yeah, ExprVarRef is the only place it's used. In a future PR (probably when I update the docs), I'll remove the Scope enum opt for a qualifier boolean.
Relevant Issues
Description
UNKNOWNAST enum variantUNKNOWNor some "OTHER" variant in the future.TruthValue.UNKtoTruthValue.UNKOWNBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.