-
Notifications
You must be signed in to change notification settings - Fork 112
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 NodeDecodingStrategy
, mirroring NodeEncodingStrategy
#45
Add NodeDecodingStrategy
, mirroring NodeEncodingStrategy
#45
Conversation
@@ -246,68 +230,4 @@ final class NodeEncodingStrategyTests: XCTestCase { | |||
("testKeyedContainer", testKeyedContainer), | |||
("testUnkeyedContainer", testUnkeyedContainer), | |||
] | |||
|
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.
The stuff being tested here is not really relevant any more.
Tests/XMLCoderTests/RJITest.swift
Outdated
|
||
func testRSS() throws { |
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 plan to re-visit this test to enable it once all issues related to it have been resolved.
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 74.98% 76.94% +1.95%
==========================================
Files 37 37
Lines 1775 1804 +29
==========================================
+ Hits 1331 1388 +57
+ Misses 444 416 -28
Continue to review full report at Codecov.
|
Sorry for the delay on this @regexident. I'm not quite sure that merging this as is would improve experience of users of Personally, I would like to avoid the breakage, so maybe we could leave the automagical behaviour and use the explicit configuration only when it's available? Making this required would add a significant amount of code to In general Here's an RFC for an API that could look like this: enum XMLCodingStrategy {
case element
case attribute
case content
}
protocol XMLDecodable: Decodable {
func decodingStrategy(for: CodingKey) -> XMLCodingStrategy
}
protocol XMLEncodable: Encodable {
func encodingStrategy(for: CodingKey) -> XMLCodingStrategy
} Then it means types that don't conform to these protocols rely on existing automagical behaviour, while types that declare these, will provide explicit configuration at the point of their declaration. What do you folks think? |
I too am far from happy with the way node-encodings are handled in both, Imho neither
Anyway, your RFC for a protocol-based solution seems like a reasonable approach to me. 👍 |
…e-decoding-strategy # Conflicts: # Sources/XMLCoder/Decoder/XMLDecoder.swift # Sources/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift # Sources/XMLCoder/Decoder/XMLUnkeyedDecodingContainer.swift # Sources/XMLCoder/Encoder/XMLEncoder.swift # Sources/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift # Sources/XMLCoder/Encoder/XMLUnkeyedEncodingContainer.swift # Tests/XMLCoderTests/BooksTest.swift # Tests/XMLCoderTests/NodeEncodingStrategyTests.swift # Tests/XMLCoderTests/RJITest.swift # XMLCoder.xcodeproj/project.pbxproj
I've merged |
The way things are right now we cannot encode an XML string like this …
… due to XMLCoder picking one
name
over the other with no way of specifying "check for aname
in the attributes" rather than "check for aname
in the elements", e.g.Also all other strategies exist as variants for encoding and decoding, never just one of them.
One change this PR introduces though is that from now on one has to specify through a
nodeDecodingStrategy
to read from an attribute. It doesn't happen automagically any more. Which it never should have, imho.We (well, "I", I guess 😅) should probably add a section on the use of
Node(En/De)codingStrategy
to the README, given that with this change (a necessary one, imho) existing code might not work any more.