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
Allow mapping-constructor-expr
to be optional in annotations
#28938
Conversation
compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java
Outdated
Show resolved
Hide resolved
@lasinicl can you please create an issue to update BBEs with this change |
.../ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/AnnotationDesugar.java
Show resolved
Hide resolved
...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java
Outdated
Show resolved
Hide resolved
...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java
Show resolved
Hide resolved
...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java
Outdated
Show resolved
Hide resolved
...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java
Outdated
Show resolved
Hide resolved
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.
@lasinicl, shall we also add content about this change to the release note file?
.../ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/AnnotationDesugar.java
Outdated
Show resolved
Hide resolved
compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java
Outdated
Show resolved
Hide resolved
...erina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java
Outdated
Show resolved
Hide resolved
|
@@ -367,4 +369,89 @@ public void testAnnotsWithConstLists() { | |||
Assert.assertEquals(element.getKind(), NodeKind.LITERAL); | |||
Assert.assertEquals(((BLangLiteral) element).value, "test"); | |||
} | |||
|
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.
Sorry, missed this previously. But shall we add some runtime tests too? See org.ballerinalang.test.annotations.AnnotationRuntimeTest. The data provider at https://github.com/ballerina-platform/ballerina-lang/blob/master/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/annotations/AnnotationRuntimeTest.java#L87 doesn't seem to be used though (the test that used that seems to have gotten removed :/), so we'll have to add that back too.
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.
Basically let's test the following,
- map type
- record type with required fields that have defaults (at runtime we can check if the default was set)
- array of maps or records
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.
Codecov Report
@@ Coverage Diff @@
## master #28938 +/- ##
============================================
+ Coverage 66.36% 66.37% +0.01%
- Complexity 33569 33590 +21
============================================
Files 2747 2747
Lines 149095 149112 +17
Branches 17996 18004 +8
============================================
+ Hits 98943 98977 +34
+ Misses 44249 44237 -12
+ Partials 5903 5898 -5
Continue to review full report at Codecov.
|
Purpose
$title.
Fixes #28885
Fixes #27498
Approach
Samples
Remarks
Check List