Skip to content
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

Always encode empty elements as KeyedBox #101

Closed
wants to merge 7 commits into from
Closed

Conversation

MaxDesiatov
Copy link
Collaborator

@MaxDesiatov MaxDesiatov commented May 11, 2019

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

@codecov
Copy link

codecov bot commented May 12, 2019

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%) ⬆️
...es/XMLCoder/Decoder/XMLDecoderImplementation.swift 65.46% <0%> (+3.48%) ⬆️
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift 77.39% <0%> (ø) ⬆️
Sources/XMLCoder/Auxiliaries/KeyedStorage.swift 100% <100%> (ø) ⬆️
...es/XMLCoder/Encoder/XMLEncoderImplementation.swift 73.04% <100%> (ø) ⬆️
...s/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift 78.09% <100%> (-6.62%) ⬇️
Sources/XMLCoder/Auxiliaries/XMLCoderElement.swift 97.17% <100%> (ø) ⬆️
Sources/XMLCoder/Auxiliaries/Box/KeyedBox.swift 100% <100%> (ø) ⬆️
Sources/XMLCoder/Encoder/XMLEncoder.swift 80% <0%> (-0.96%) ⬇️
... 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.

@MaxDesiatov MaxDesiatov marked this pull request as ready for review May 12, 2019 17:28
@grin
Copy link

grin commented May 16, 2019

@MaxDesiatov a quick question, is 237cc28 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 MaxDesiatov added the work in progress Not ready to merge or to be closed label May 16, 2019
@MaxDesiatov
Copy link
Collaborator Author

@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 added a commit that referenced this pull request May 22, 2019
`UnkeyedBox` is a wrapper for `[Box]`, the assumption is that adding direct `Box` conformance to `[Box]` would simplify things a bit. Also, `KeyedStorage` now always stores `[Box]` per key without any special handling for single items per key to simplify this even more. In addition, order of values is preserved for all keys as required for #91 and #25.

This should also unblock #101 providing unified handling for elements without any content and attributes.

By pure serendipity this also fixes tests introduced in #38.

* Replace UnkeyedBox with Array, refine KeyedStorage
* Fix more of the broken tests
* One unfixed test left in benchmarks
* Single out failing benchmark in a separate test
* Fix all tests 🎉
* Fix compiler warning
* Fix Xcode 10.1 compilation error
* Remove unused AnyArray protocol
* Remove unused elementType function
* Simplify code to improve test coverage
…ullbox

# Conflicts:
#	Sources/XMLCoder/Auxiliaries/KeyedStorage.swift
#	Sources/XMLCoder/Decoder/XMLDecoderImplementation.swift
#	Sources/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift
#	Sources/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift
#	Tests/XMLCoderTests/Box/KeyedBoxTests.swift
#	Tests/XMLCoderTests/Box/UnkeyedBoxTests.swift
#	XMLCoder.xcodeproj/project.pbxproj
@pj4533 pj4533 mentioned this pull request May 22, 2019
@MaxDesiatov
Copy link
Collaborator Author

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.

@MaxDesiatov MaxDesiatov deleted the avoid-nullbox branch April 9, 2020 20:52
arjungupta0107 pushed a commit to salido/XMLCoder that referenced this pull request Jun 26, 2020
`UnkeyedBox` is a wrapper for `[Box]`, the assumption is that adding direct `Box` conformance to `[Box]` would simplify things a bit. Also, `KeyedStorage` now always stores `[Box]` per key without any special handling for single items per key to simplify this even more. In addition, order of values is preserved for all keys as required for CoreOffice#91 and CoreOffice#25.

This should also unblock CoreOffice#101 providing unified handling for elements without any content and attributes.

By pure serendipity this also fixes tests introduced in CoreOffice#38.

* Replace UnkeyedBox with Array, refine KeyedStorage
* Fix more of the broken tests
* One unfixed test left in benchmarks
* Single out failing benchmark in a separate test
* Fix all tests 🎉
* Fix compiler warning
* Fix Xcode 10.1 compilation error
* Remove unused AnyArray protocol
* Remove unused elementType function
* Simplify code to improve test coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress Not ready to merge or to be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants