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

Overhaul internal representation, replacing NS… with …Box types #19

Merged
merged 22 commits into from
Dec 20, 2018
Merged

Conversation

regexident
Copy link
Contributor

@regexident regexident commented Dec 18, 2018

This PR removes any remaining use of NS…Array/NS…Dictionary/NSNumber/NSDecimalNumber/NSNull/… (as storage) from the code-base.

It introduces an internal type …

internal enum Box {
    case null(NullBox)
    case bool(BoolBox)
    case decimal(DecimalBox)
    case signedInteger(SignedIntegerBox)
    case unsignedInteger(UnsignedIntegerBox)
    case floatingPoint(FloatingPointBox)
    case string(StringBox)
    case array(ArrayBox)
    case dictionary(DictionaryBox)
}

… as well as accompanying variant box types, replacing use of Any/NS….

👷🏻‍♀️It improves type-safety by reducing use of Any (from 60 down to 19 occurrences) as well as NSObject (from 37 down to 1 occurrence).

🏗It further more levels the field for improvements/additions such as support for order-preserving elements & attributes.

💡Thanks to encapsulation we can now safely change the inner logic of DictionaryBox to retain insertion order.

Edit:

We ended up replacing aforementioned enum Box with a protocol:

protocol Box {
    var isNull: Bool { get }
    var isFragment: Bool { get }
    
    func xmlString() -> String?
}

@codecov
Copy link

codecov bot commented Dec 18, 2018

Codecov Report

Merging #19 into master will increase coverage by 9.37%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   39.36%   48.74%   +9.37%     
==========================================
  Files          14       28      +14     
  Lines        1443     1629     +186     
==========================================
+ Hits          568      794     +226     
+ Misses        875      835      -40
Impacted Files Coverage Δ
...es/XMLCoder/Auxiliaries/ISO8601DateFormatter.swift 100% <ø> (ø)
Sources/XMLCoder/Auxiliaries/XMLKey.swift 25% <ø> (ø)
...rces/XMLCoder/Decoder/DecodingErrorExtension.swift 0% <0%> (ø) ⬆️
...urces/XMLCoder/Encoder/XMLReferencingEncoder.swift 0% <0%> (ø) ⬆️
...urces/XMLCoder/Auxiliaries/String+Extensions.swift 100% <100%> (ø)
Sources/XMLCoder/Decoder/XMLDecodingStorage.swift 81.81% <100%> (ø) ⬆️
...XMLCoder/Encoder/XMLUnkeyedEncodingContainer.swift 20% <11.11%> (-3.26%) ⬇️
Sources/XMLCoder/Box/NullBox.swift 27.27% <27.27%> (ø)
Sources/XMLCoder/Decoder/XMLDecoder.swift 34.75% <33.33%> (-0.05%) ⬇️
...s/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift 38.86% <45.83%> (+10.93%) ⬆️
... and 36 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 863b160...aa25f45. Read the comment docs.

@regexident regexident changed the title Added explicit …Box types, replacing implicit use of NS… Added explicit …Box types, replacing use of NS… Dec 18, 2018
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @regexident!

This is definitely a change in the right direction, but I think that we should use a Box protocol instead of a Box enum, which would remove quite a few switch statements that delegate to implementations of the same shape anyway.

Also, I'm not quite sure why some of the new containers are reference types and not value types as standard collections. In fact, you probably could avoid introducing those collections at all by adding conformance to Box protocol to standard Array and Dictionary if I convinced you that Box protocol is a better approach.

I look forward to your feedback on my proposal with Box protocol and hope that we can find the best solution that replaces legacy Foundation types from Objective-C days.

Sources/XMLCoder/Auxiliaries/XMLElement.swift Show resolved Hide resolved
Sources/XMLCoder/Auxiliaries/XMLElement.swift Show resolved Hide resolved
Sources/XMLCoder/Auxiliaries/XMLElement.swift Show resolved Hide resolved
Sources/XMLCoder/Auxiliaries/XMLElement.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Auxiliaries/XMLElement.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/Box.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/Box.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/Box.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/Box.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/DictionaryBox.swift Outdated Show resolved Hide resolved
@MaxDesiatov MaxDesiatov changed the title Added explicit …Box types, replacing use of NS… Add explicit …Box types, replacing use of NS… Dec 18, 2018
@regexident
Copy link
Contributor Author

regexident commented Dec 18, 2018

The problems I see with turning Box into a protocol are these:

The protocol(s) would have to look something like this to work:

protocol AnyBoxProtocol: Equatable {
    var xmlString: String? { get }
    var isNull: Bool { get }
    var isFragment: Bool { get }
}

protocol BoxProtocol: AnyBoxProtocol {
    associatedtype Unboxed
    
    init(_ unboxed: Unboxed)
    
    func unbox() -> Unboxed
}

But in the case of SignedIntegerBox/UnsignedIntegerBox/FloatingPointBox the BoxProtocol couldn't even be implemented properly as their init(_:) and func unbox differ significantly due to covering whole type families, not just a single wrapped type.

If Swift allowed for multiple protocol conformances we'd be fine. Alas it doesn't.

Another benefit of using a enum is that we get a guarantee for exhaustive switches.
Adding a new box type some time in the future is far less brittle that way, than it is with type erasure (be it Any or protocol Box).

@regexident
Copy link
Contributor Author

Also could Apple please get rid of the plague that is . xcodeproj already?
Those merge conflicts are the worst. It's almost 2019, damnit! 😩

@MaxDesiatov
Copy link
Collaborator

Great point, I think we should consider moving all custom settings to Package.xcconfig like I've done in CoreXLSX and get rid of .xcodeproj altogether in the repository encouraging users to run swift package generate-xcodeproj --xcconfig-overrides Package.xcconfig on their side when needed. The only problem is baselines which are stored within .xcodeproj and I'm not sure how useful benchmarks are without baselines... 🤔

@regexident
Copy link
Contributor Author

Commit 7fe7984 just removed enum Box in favor of protocol Box. 🙂

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, except the commented out code and use of internal, where I think it's a good chance to clean it up in this PR with the changes touching relevant code

Sources/XMLCoder/Box/ArrayBox.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/DictionaryBox.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/UnsignedIntegerBox.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Auxiliaries/XMLHeader.swift Show resolved Hide resolved
Sources/XMLCoder/Auxiliaries/XMLHeader.swift Show resolved Hide resolved
Sources/XMLCoder/Box/BoolBox.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/ArrayBox.swift Outdated Show resolved Hide resolved
Sources/XMLCoder/Box/DictionaryBox.swift Outdated Show resolved Hide resolved
@regexident regexident changed the title Add explicit …Box types, replacing use of NS… Overhaul internal representation, replacing NS… with …Box types Dec 19, 2018
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff, but there are a few comments left with regards to test cases, at least Constructor typealias was unused in one place and it also looked equivalent to FromXMLString. It also took me some time to understand what FromXMLString is for and I got some clarity only after I started reviewing the rest of the tests, so this testing approach needs to be documented or somehow simplified in my opinion.

Tests/XMLCoderTests/Box/DataBoxTests.swift Outdated Show resolved Hide resolved
Tests/XMLCoderTests/Box/DataBoxTests.swift Outdated Show resolved Hide resolved
Tests/XMLCoderTests/Box/DataBoxTests.swift Show resolved Hide resolved
MaxDesiatov
MaxDesiatov previously approved these changes Dec 20, 2018
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @regexident, this is great! @hodovani, do you have any questions or comments about this PR? Otherwise this can be merged when .xcodeproj conflict is resolved.

hodovani
hodovani previously approved these changes Dec 20, 2018
@regexident regexident dismissed stale reviews from hodovani and MaxDesiatov via aa25f45 December 20, 2018 11:39
@regexident
Copy link
Contributor Author

regexident commented Dec 20, 2018

Rebased onto master. (no additional changes, other than fixing merge conflicts of .xcodeproj.)

@MaxDesiatov MaxDesiatov merged commit ad96c5e into CoreOffice:master Dec 20, 2018
@regexident
Copy link
Contributor Author

Phew, this PR ended up quite bigger than anticipated, but it's worth it, I think.

The separation and encapsulation, as well as addition of finely-grained unit tests should give us a good and solid foundation for future improvements and bug fixes.

Thanks for your comments, @MaxDesiatov!

@regexident regexident deleted the boxes branch December 20, 2018 11:52
jsbean added a commit to bwetherfield/XMLCoder that referenced this pull request Jul 16, 2019
bwetherfield pushed a commit to bwetherfield/XMLCoder that referenced this pull request Jul 26, 2019
bwetherfield pushed a commit to bwetherfield/XMLCoder that referenced this pull request Jul 26, 2019
Add benchmark baselines

Pull in tests (CoreOffice#11)

Add Decoding support for choice elements (CoreOffice#15)

Fix indentation (CoreOffice#16)

Replace usage of XCTUnrwap (CoreOffice#19)

Add nested choice tests (CoreOffice#18)

Add falsely passing double array roundtrip test (CoreOffice#17)
bwetherfield pushed a commit to bwetherfield/XMLCoder that referenced this pull request Jul 27, 2019
Add benchmark baselines

Pull in tests (CoreOffice#11)

Add Decoding support for choice elements (CoreOffice#15)

Fix indentation (CoreOffice#16)

Replace usage of XCTUnrwap (CoreOffice#19)

Add nested choice tests (CoreOffice#18)

Add falsely passing double array roundtrip test (CoreOffice#17)
bwetherfield pushed a commit to bwetherfield/XMLCoder that referenced this pull request Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants