-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add support for root attributes propagation #160
Conversation
Hi @portellaa, thanks for the PR! Would you be able to give an example of XML and their corresponding codable types that show how exactly this would be a useful addition? Also, these changes are not tested and I'm wary of decreasing the test coverage of the library, could those examples then be adapted as new test cases submitted in this same PR? |
hi @MaxDesiatov of course, Imagine the following model: struct ServiceRequest: Codable {
let type: String
let id: String
let element1: String
let element2: String
} Now imagine that the API requires that the xml generated, contains the namespace (xmlns), prefix (xmlns:xsi) and schema (xmlns:xsd), in the root element, you would have to generate something like this: <?xml version="1.0"?>
<ServiceRequest xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema" Type="Test" ID="id1" xmlns="http://www.nrf- arts.org/IXRetail/namespace">
<Element1>Test</Element1>
<Element2>Test</Element2>
</CardServiceRequest> I don't want (and/or need), that my model contains the Does it sounds right to you? Cheers 🍻 |
Ok, that makes sense in general, thanks for the clarification. This only touches encoding though, how would you like those attributes to be handled on the decoding side? |
I can handle them the same way, provide a Do you have an idea? I just don't want the api to get weird of out of the "standard" with that attributes thing? |
Should i create a test case for those tests? Or should i include them in the one of the existing ones? 🤔 |
That makes sense. I'm just wondering, if in the future this is to be implemented on the decoding side, we'd probably want the API to be consistent for both encoding and decoding and hopefully also intuitive enough. If we can anticipate how it would look for decoding, making it consistent would be much easier. Adding a test case probably with the example you gave would be great, thank you! |
5514a60
to
d24ff75
Compare
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 72.84% 73.06% +0.21%
==========================================
Files 43 43
Lines 2287 2309 +22
==========================================
+ Hits 1666 1687 +21
- Misses 621 622 +1
Continue to review full report at Codecov.
|
d24ff75
to
cc14da0
Compare
Sorry i was working on a different thing and didn't had time to finish this. |
8ed6599
to
65c6707
Compare
import XCTest | ||
@testable import XMLCoder | ||
|
||
class RootLevetExtraAttributesTests: XCTestCase { |
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.
class RootLevetExtraAttributesTests: XCTestCase { | |
final class RootLevetExtraAttributesTests: XCTestCase { |
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.
i completely agree with this, but i checked the other classes and so i followed the same. changed 😊 👍
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 about that, the library does have some inconsistencies which is caused more by the lack of my attention than anything else 🙈
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.
that's normal 🤷 i should have followed what i usually do, but i prefer to respect the "style" 😅
@@ -318,28 +322,35 @@ open class XMLEncoder { | |||
/// | |||
/// - parameter value: The value to encode. | |||
/// - parameter withRootKey: the key used to wrap the encoded values. | |||
/// - parameter rootAttributes: the list of attributes to be added to root node |
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.
/// - parameter rootAttributes: the list of attributes to be added to root node | |
/// - parameter rootAttributes: the list of attributes to be added to the root node |
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.
ups thanks 🙈
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.
Apart from minor nitpicks, the code looks good to me as such, thank you for adding the tests! I have to say, I'm still a bit ambiguous about the API. Given that the main use case is in handling xmlns
attributes, I wonder if there's a better way to do this, maybe more focused on xmlns
functionality itself in conjunction with how namespace prefixes are handled, but I don't have enough experience with handling XML namespaces to say what API would work better.
I'd be happy to hear if frequent contributors and users of downstream dependencies of XMLCoder have any opinions (cc @bwetherfield)
To pose this from a different perspective, are there any other root attributes other than If only possible root attributes a user would like to handle are the (I don't have an answer to these questions, I'm just looking for more feedback before potentially merging it as is) |
i agree that this is not the best way to handle this. <root xmlns:h="http://www.w3.org/TR/html4/"
xmlns:f="https://www.w3schools.com/furniture">
<h:table>
<h:tr>
<h:td>Apples</h:td>
<h:td>Bananas</h:td>
</h:tr>
</h:table>
<f:table>
<f:name>African Coffee Table</f:name>
<f:width>80</f:width>
<f:length>120</f:length>
</f:table>
</root> as far i understood from https://www.w3.org/TR/xmlschema-1/ those xsd (or xs) and xsi are standard prefixes commonly used, where i remember from the times i use to use SOAP and xml envelopes (12y ago), those are attributes were very important, because they were used for validation of the document, i believe they may not be that important right now since nowadays most of the people and services users JSON, protobuf and so on, so... i've be reading more on this subject and more than before, now i think this is a bad addition because it will induce people in error if they expect schema parsing and validation in the coders. that said, feel free to not accept this. at least it can be used for discussion, but probably move this to an issue is the best idea. |
@portellaa thanks for the write up and the contribution! I'll keep this open for a few days for anyone interested to post their feedback, otherwise I'll merge it if no strong reasons against it are found by that time 🙂 |
no worries, in the meanwhile i will try to found a time where i can work on a solution to integrate xmlns properly, with validation and parsing 🙂 more information regarding xsd and xsi i will keep this here to use later as a guidance in the implementation 😊 |
Please also have a look at the existing Again, this needs some design and research work, so if you would like to avoid that, we could keep the current |
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.
Overall, LGTM! Just saw a few possible redundancies.
@@ -348,30 +350,30 @@ extension XMLCoderElement { | |||
self.init(key: key, elements: elements, attributes: attributes) | |||
} | |||
|
|||
init(key: String, box: SimpleBox) { | |||
init(key: String, box: SimpleBox, attributes: [Attribute] = []) { |
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.
I think you can do without the attributes in the SimpleBox
initializer!
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.
yes yes nice catch 👍
rootAttributes: rootAttributes, | ||
userInfo: userInfo) | ||
} | ||
|
||
public var rootAttributes: [String: String] = [:] | ||
|
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.
Are these still needed?
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.
no we don't. thank you 😊
@@ -293,6 +293,7 @@ open class XMLEncoder { | |||
let keyEncodingStrategy: KeyEncodingStrategy | |||
let nodeEncodingStrategy: NodeEncodingStrategy | |||
let stringEncodingStrategy: StringEncodingStrategy | |||
let rootAttributes: [String: String] |
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.
Delete?
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.
thank you for notice 🙏
3a126c8
to
1b7cb2a
Compare
I did the pipelines failed? they don't show info about what happened. it seems they didn't even run. |
Thanks, seems like there was an intermitten problem with GitHub Actions, I've restarted the jobs, the Danger job is still failing, but it's not critical in any way. BTW, in this project and most of the projects that I maintain there's no need to squash and/or force push, PRs can contain whatever makes sense and easier to manage for the purpose of a PR, including merge commits. They are always merged with "Squash and merge" after that to maintain linear history in |
it's an habit sorry. the merge strategy it's not visible so i prefer to always squash, but i will have in mind that i don't need to do it on your repos 😃 |
This will be released as a part of 0.11, thanks again @portellaa! |
Sometimes we want some attributes in the root node of our xml that don't should or need to be in the model, for example the attributes for the namespace or schema. This pull request uses the existing attributes in the boxes to add those attributes to the root node. Also, we can now infer the root node name instead of providing it in the encode method, making it optional in the API.
Sometimes we want some attributes in the root node of our xml that don't should or need to be in the model, for example the attributes for the namespace or schema.
This pull request uses the existing attributes in the boxes to add those attributes to the root node.
Also, we can now infer the root node name instead of providing it in the encode method, making it optional in the API.
I'm not sure if this is the best place, or add them as a configuration for the coders instead of use them all the time we call the
encode
method.It would be nice if those attributes were moved to the coder configuration, so we could have the clean coding methods and looking more like the Apple JSON coders. What you think?