CoreOffice / XMLCoder

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

Add Linux support #117

Closed drewag closed 5 years ago

drewag commented 5 years ago

Only a few minor changes needed to be made for this to work properly on Linux.

drewag commented 5 years ago

Thanks for the feedback. Can I just confirm that supporting Linux moving forward is a goal worth pursuing before I make these changes?

MaxDesiatov commented 5 years ago

@drewag yes, absolutely, I'd be super happy to merge and maintain Linux support, as long as we can see a clear path towards unifying the behaviour across all platforms. I previously did plan to add support for Linux to XMLCoder, but I wasn't sure if Foundation had the feature parity on Linux, especially in terms of XML namespaces support. I'm fine with having small inconsistencies in master and maybe even tagged versions, but this has to be clearly documented and tested on CI.

drewag commented 5 years ago

Ok great, I'll work on it.

drewag commented 5 years ago

There are two failing tests on Linux if I revert to Swift 4.2. Looking into what the problems are now.

drewag commented 5 years ago

One failed test is NoteTest.testInvalidXML. The 4.2 parser fails to catch the "Ffrom" inconsistency. Not sure what to do about that except ignore that test on versions before 5.0.

The harder one is ErrorContextTest.testErrorContext.

There we are getting the wrong error type:

typeMismatch(Swift.Dictionary<Swift.String, Any>, Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "value", intValue: nil)], debugDescription: "Expected to decode Dictionary<String, Any> but found SharedBox<Array> instead.", underlyingError: nil))

That one is a bit above my pay grade. Any ideas?

MaxDesiatov commented 5 years ago

Are both of those errors reproducible on 4.2, but only on Linux? If so, we could keep 4.2 compatibility on Apple platforms, but require 5.0 on Linux. This would mean not testing 4.2 Linux on CI and mentioning that Linux support requires 5.0 in README.md

drewag commented 5 years ago

Yup that's correct, all tests pass on Linux with Swift 5. I'll update the README and then we should be ready to merge.

drewag commented 5 years ago

Sorry for all the commits (Azure Pipelines is new to me). This should finally be ready to merge.