Fix more Linux issues #93
Conversation
|
@vincentneo By the way, if/when you merge this, I would appreciate it if you could tag a new release so that I don't have to make the Swift Package Manager depend on either |
|
So sorry @Gerzer. I missed out on this. Will review soon. |
Classes/DateTimeParsers.swift
Outdated
| @@ -55,15 +53,8 @@ final class GPXDateParser { | |||
| components.hour = hour.pointee | |||
| components.month = month.pointee | |||
| components.second = second.pointee | |||
|
|
|||
There was a problem hiding this comment.
Hi @Gerzer, I had modified a bit of the Test case code on my end, to include date parse check, but it seems like as compared to master branch, this branch fails to parse date, becoming nil.
This can be seen in the latest commit of this branch. Check testParseDualLinksWithURL(). To compare, can replace that function over to master branch.
I assume there is something to do with the code removal in the DateTimeParsers.
If you're able to rectify this issue without a performance loss, it should be good to go other wise. Thank you!
There was a problem hiding this comment.
The tests all crash for me because the GPXTest-TrackPointOnly bundle resource isn't found. How do I resolve this so that I can reproduce the actual test failure?
There was a problem hiding this comment.
Hi @Gerzer,
The files are in the Test directory https://github.com/vincentneo/CoreGPX/tree/master/Example/Tests
There was a problem hiding this comment.
The issue is that some of the calendar functions that you’re using aren’t available on Linux. Would it be acceptable to keep your existing code but to fall back on the less sophisticated code when compiling for Linux? The tests would pass on macOS, but they would continue to fail on Linux. I could modify those tests as well as the documentation to reflect the differences in behavior. From what I can tell, the issue occurs when trying to parse non-standard date strings.
There was a problem hiding this comment.
The current date parser was used as the default was fairly slow, when considering the scale. (like parsing 1000s of dates)
I reckon we could do a fallback to DateFormatter for linux? (assuming DateFormatter is available?)
There was a problem hiding this comment.
I apologize for the delay on moving forward with this. I was in a position for a few months in which I wasn’t able to contribute to open-source projects. However, that situation is over, so I’ll follow up here soon.
|
looks good to me! Thank you so much @Gerzer! |
I've fixed a runtime issue with date parsing on Linux and addressed some buildtime warnings.