Closed bwetherfield closed 3 years ago
Update: I'm leaving in DynamicNodeEncoding for now as a workaround for the one test case that is causing me problems, but otherwise having the encoder use property attributes to determine node types.
Merging #192 (be5ad9b) into master (b8fa7fe) will decrease coverage by
0.29%
. The diff coverage is71.27%
.
@@ Coverage Diff @@
## master #192 +/- ##
==========================================
- Coverage 73.80% 73.50% -0.30%
==========================================
Files 43 46 +3
Lines 2355 2404 +49
==========================================
+ Hits 1738 1767 +29
- Misses 617 637 +20
Impacted Files | Coverage Δ | |
---|---|---|
Sources/XMLCoder/Auxiliaries/Element.swift | 0.00% <0.00%> (ø) |
|
Sources/XMLCoder/Auxiliaries/XMLCoderElement.swift | 96.34% <ø> (ø) |
|
...es/XMLCoder/Decoder/XMLDecoderImplementation.swift | 67.55% <ø> (ø) |
|
...es/XMLCoder/Encoder/XMLEncoderImplementation.swift | 76.57% <ø> (ø) |
|
...ces/XMLCoder/Auxiliaries/ElementAndAttribute.swift | 33.33% <33.33%> (ø) |
|
Sources/XMLCoder/Auxiliaries/Attribute.swift | 60.00% <60.00%> (ø) |
|
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift | 73.15% <72.00%> (-0.87%) |
:arrow_down: |
...s/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift | 84.57% <93.54%> (+2.76%) |
:arrow_up: |
Sources/XMLCoder/Auxiliaries/XMLStackParser.swift | 92.77% <100.00%> (-0.78%) |
:arrow_down: |
Sources/XMLCoder/Decoder/XMLDecoder.swift | 76.64% <100.00%> (ø) |
|
... and 4 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update b8fa7fe...be5ad9b. Read the comment docs.
I like the idea of Both
being default behavior, as that name is not the nicest to look at. In terms of default encoding, we have previously had .element
as default (in the other implementation). So that's the only question mark. We will have a lot more @XMLElement
's to add, as this is pretty common. Not necessarily a bad thing to be explicit that way.
re the naming, when I name the property wrapper XMLElement
I get a funny error from the compiler (one of the reasons I stuck Node
on the end):
Unknown attribute 'XMLElement'
Must be a naming conflict somewhere I think. If I change to @XMLCoder.XMLElement
the compiler is happy. So maybe we go for @XMLCoder.Element
and XMLCoder.Attribute
instead? What do you think, @MaxDesiatov ?
I think we are probably getting a name conflict with XMLElement
here
@Element
and @Attribute
LGTM, and you're right, those can be disambiguated in client code as @XMLCoder.Element
and @XMLCoder.Attribute
if needed.
With regards to the third option, we have some assymetry here:
public enum NodeDecoding {
case attribute
case element
case elementOrAttribute
}
Interestingly enough NodeDecoding
doesn't have a default
static property.
public enum NodeEncoding {
case attribute
case element
case both
public static let `default`: NodeEncoding = .element
}
Thus @Both
makes sense only for encoding. I don't have a good suggestion for it, especially with the assymetry. It could be @BothAttributeAndElement
, which is ugly, but explicit, and maybe then it could behave symmetrically for both encoding and decoding? As in, it would work as .elementOrAttribute
for decoding and .both
for encoding. BTW, I'm not a native English speaker, so please feel free to suggest a better name 😄
I've just rolled back the existing DynamicNodeEncoding
tests. Mostly wanted to see whether things would work quickly. Now we can add separate tests conditional on swift(>= 5.1)
I am wondering if .elementAndAttribute
could be a more descriptive name than .both
(while also serving symmetry a little better). Then @ElementAndOrAttribute
could be an option, but I don't know how clear that actually is! What do you think?
Maybe @ElementAndAttribute
? In the decoding case it would read as "looking for value in both element and attribute" and in the encoding case as "writing the value to both element and attribute". What do you think?
Yeah, I was just thinking that. I think that's a great idea!
Ah, now it looks like Attribute
has some naming conflicts (within the codebase)! Would you be happy with AttributeTag
, AttributeFlag
or AttributeDecorator
or perhaps something else...?
Interesting, what does plain Attribute
conflict with? Maybe that's something internal that could be renamed?
Right, there is Attribute
that sits alongside XMLCoderElement
so could
very sensibly be renamed XMLCoderAttribute
. That allows us to use
@Attribute
or @XMLCoder.Attribute
without problems. Does that work?
On Wed, 10 Jun 2020 at 12:14, Max Desiatov notifications@github.com wrote:
Interesting, what does plain Attribute conflict with? Maybe that's something internal that could be renamed?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/MaxDesiatov/XMLCoder/pull/192#issuecomment-642204043, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZ3VKUVRP32ROHGUYLZPD3RV7LPZANCNFSM4NYA4XUQ .
Absolutely! In fact, maybe we could get away with moving that
struct Attribute: Equatable {
let key: String
let value: String
}
into the body of XMLCoderElement
, which would change the fully-qualified name to XMLCoderElement.Attribute
, I hope that would work. And it kinda makes sense since stand-alone attributes can't exist outside of elements.
Ah, nice. Yeah, I'll give that a try!
Hi @MaxDesiatov and @bwetherfield . I have an XML full of attributes. But also need to decode the older flavour of that XML which has everything as elements instead. Do I need this PR? Any chance for examples? Cheers.
@transat either with this PR or without it you'll probably have to create two separate model types. In master
or in the latest tagged release you'll have to use dynamic node coding protocols for it if that makes sense?
Thanks @MaxDesiatov Yes, I've managed to successfully decode the attribute-only version of the file and will give the element-only version a go next.
Could I keep the same model and ostensibly do something like this?
static func nodeDecoding(for key: CodingKey) -> XMLDecoder.NodeDecoding {
if ( xmlFileRootElementToString == "sound" ){
return .attribute
}
else {
return .element
}
}
BTW, is there a preferred avenue for me to seek assistance if I get stuck? I don't want to spam this issue. For reference, the 2 versions of the files I'm currently trying to decode are:
Yes, I think your suggestion with one model type and branching with xmlFileRootElementToString
could work, although I'm not sure how easy it will be to get that value passed to that function.
Please feel free to create a separate issue if you find any problems.
Hi @bwetherfield, would you be able to finish this up? If you're currently busy, I hope I can pick it up at some point.
Hi @MaxDesiatov. Sorry - forgot about this! I took a look and got close to a satisfactory solution I think. I can't get ExpressibleByNilLiteral
conformance working for now, which is kind of a deal breaker. That said, CI is all red (tests clear locally) - do you know what's going on there?
Thanks!
Ci failures are caused by SwiftLint, which complains about force casts. If those can't be avoided you can add something like // swiftlint:dissable:next force_cast
on a line that preceeds a force cast.
OK, I gave that a try - I don't know of any other way to do Expressible...
conformance, so I tried to enable the forcecasts. Still may be going red.
This is probably the most work I can do on this for the next while as I'm tied up in other projects. I'm happy to close or have you try to tackle some, @MaxDesiatov. Thanks and sorry for the limited bandwidth!
No problem at all, I can pick it up from here. Thanks for you work!
Great job with this @MaxDesiatov. I always learn a lot from reading over your code! Thanks for getting this PR over the line.
Are there any instructions for using this? Cheers!
We've discussed some basic use in https://github.com/MaxDesiatov/XMLCoder/issues/189, but since this feature is only available in the main
branch, I haven't added any docs to README.md
yet.
Resolves #189