-
Notifications
You must be signed in to change notification settings - Fork 753
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
Support new options in xml conversion #42440
Support new options in xml conversion #42440
Conversation
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
Build failed due to the checkstyle failure, can you take a look? |
This PR did not include tests. Right? Shall we add considerable amount of tests for the changes. |
@prakanth97 @mindula Do we need a config for explicitly adding attributes? I guess, currently we add the annotation by default. But runtime logic doesn't depend on this annotation. So someone can decide to omit it. Having a way to configure the behavior useful. But this can later. |
misc/xml-to-record-converter/src/test/resources/ballerina/sample_15.bal
Outdated
Show resolved
Hide resolved
Are you suggesting to add another configuration option to include attributes or not? example:
Without attributes
With attributes
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42440 +/- ##
=============================================
+ Coverage 0.00% 76.74% +76.74%
- Complexity 0 53828 +53828
=============================================
Files 9 2913 +2904
Lines 35 203404 +203369
Branches 0 26525 +26525
=============================================
+ Hits 0 156107 +156107
- Misses 35 38764 +38729
- Partials 0 8533 +8533 ☔ View full report in Codecov by Sentry. |
the name space is applied to all the elements and the attribute in the child of book and element book as well. Hence the expected record should be
But what we generate at the moment is
This seems wrong. |
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Show resolved
Hide resolved
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
...o-record-converter/src/main/java/io/ballerina/xmltorecordconverter/XMLToRecordConverter.java
Outdated
Show resolved
Hide resolved
string xmlns\:xsi = "http://www.w3.org/2001/XMLSchema-instance"; | ||
@xmldata:Attribute | ||
string xsi\:nil; | ||
string nil; |
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.
We did not add the namespace annotation here, seems wrong
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.
Shall we add a example where we have namespace for the attribute?
Shall we add few tests for disabling namespace? |
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.
LGTM
c4cd054
into
ballerina-platform:master
Purpose
$title.
Fixes #42385
Approach
Samples
Remarks
Check List