-
Notifications
You must be signed in to change notification settings - Fork 108
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
Update doc comments, add PropertyWrappersTest
#246
Conversation
Hi @CoreOffice/xmlcoder-team, would you have a moment to take a look? I wanted to make sure we're on the same page in terms of documentation, so your PR review here would be appreciated. Thanks! |
case element | ||
/// Decodes a node from either elements of form `<nodeName>value</nodeName>` or attributes | ||
//// of form `nodeName="value"`. |
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.
Do we guarantee the order in which the check is made?
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.
Great catch, I've clarified it in the comment
|
||
func testDecode() throws { | ||
let decoder = XMLDecoder() | ||
let decodedBookBoth = try decoder.decode(Book.self, from: Data(bookAuthorElementAndAttributeXML.utf8)) |
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.
Shouldn’t we also add a from string decoding API, if one is seemingly lacking. That is not a change request, but a question
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 this would be a sensible addition and raises a question why JSONDecoder
haven't got anything like this by now? XMLCoder was modelled after JSONDecoder
and JSONEncoder
APIs.
What are the odds that people currently rely on this API? I’m not exactly for a breaking change, but I wonder what the impact would be |
If you're referring to In theory, I could expect someone conforming to this protocol similarly by accident, but even doing that would require some conscious effort on their side, and they shouldn't be doing this in the first place. Since XMLCoder 1.0 haven't been tagged yet so far, I think it's acceptable to introduce this breaking change for the sake of having a cleaner API. |
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.
Considering the internal API is not interesting to implement, and we're pre 1.0, I'm happy removing it in a minor. The rest is great!
When viewing documentation published at https://swiftpackageindex.com/CoreOffice/XMLCoder/main/documentation/xmlcoder, I noticed that some of the doc comments are missing, and we made some of the protocols
public
by mistake. I've added doc comments with example code and update those protocols accordingly.Additionally, tests and a section in
README.md
for recently added property wrappers were missing, that's fixed as well.