search

CoreOffice / XMLCoder

Easy XML parsing using Codable protocols in Swift
https://coreoffice.github.io/XMLCoder/
MIT License
801 stars 112 forks source link

Always encode empty elements as KeyedBox #101

Closed MaxDesiatov closed 5 years ago

MaxDesiatov commented 5 years ago

This fixes decoding of empty elements in certain cases that weren't previously covered, specifically MaxDesiatov/CoreXLSX#64.

codecov[bot] commented 5 years ago

Codecov Report

Merging #101 into master will increase coverage by 1.9%. The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #101     +/-   ##
=========================================
+ Coverage   75.82%   77.72%   +1.9%     
=========================================
  Files          38       37      -1     
  Lines        2039     2088     +49     
=========================================
+ Hits         1546     1623     +77     
+ Misses        493      465     -28
Impacted Files Coverage Δ
...rces/XMLCoder/Decoder/DecodingErrorExtension.swift 66.66% <ø> (+66.66%) :arrow_up:
...es/XMLCoder/Decoder/XMLDecoderImplementation.swift 65.46% <0%> (+3.48%) :arrow_up:
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift 77.39% <0%> (ø) :arrow_up:
Sources/XMLCoder/Auxiliaries/KeyedStorage.swift 100% <100%> (ø) :arrow_up:
...es/XMLCoder/Encoder/XMLEncoderImplementation.swift 73.04% <100%> (ø) :arrow_up:
...s/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift 78.09% <100%> (-6.62%) :arrow_down:
Sources/XMLCoder/Auxiliaries/XMLCoderElement.swift 97.17% <100%> (ø) :arrow_up:
Sources/XMLCoder/Auxiliaries/Box/KeyedBox.swift 100% <100%> (ø) :arrow_up:
Sources/XMLCoder/Encoder/XMLEncoder.swift 80% <0%> (-0.96%) :arrow_down:
... and 9 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 eb22efd...45da9e4. Read the comment docs.

grin commented 5 years ago

@MaxDesiatov a quick question, is https://github.com/MaxDesiatov/XMLCoder/pull/102/commits/237cc28f11f4506b60e30af28f4cb076e30ee799 required for this to work? I'm getting a bunch of non-trivial conflicts when trying to merge it:

Zrzut ekranu 2019-05-16 o 12 50 25

also, it doesn't seem to be working without it, at least for me.

MaxDesiatov commented 5 years ago

@grin you're right, this PR is blocked by #102, but some of the tests are still failing, I hope to get to it some time next week. Thanks for your comment, I've just added the "work in progress" label

MaxDesiatov commented 5 years ago

As was highlighted in #103 a nice workaround is to make the array element optional. While this PR increases test coverage by deleting NullBox, the benchmarks show that for corresponding cases performance degrades by at least 30%. I personally would prefer to increase the test coverage by adding more tests that don't kill the performance, rather than deleting code that makes the code more performant. Closing this and I hope we can find a better solution in the future.