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

iOS-393 Move NYPLReachabilityManager class from Simplified-iOS repo #12

Merged
merged 1 commit into from
May 17, 2022

Conversation

ErnestFan
Copy link
Contributor

What's this do?
Move NYPLReachabilityManager so it is accessible from NYPLAudiobookToolkit

Why are we doing this? (w/ JIRA link if applicable)
iOS-393

How should this be tested? / Do these changes have associated tests?
N/A

Dependencies for merging? Releasing to production?
N/A

Does this include changes that require a new SimplyE/Open eBooks build for QA?
N/A

Has the application documentation been updated for these changes?
N/A

Did someone actually run this code to verify it works?
@ErnestFan

@ErnestFan ErnestFan requested a review from ettore May 16, 2022 23:41
@ErnestFan
Copy link
Contributor Author

There is another related class NYPLReachability in Simplified-iOS repo, I didn't move that because that's for constantly monitoring the reachability status which is not needed for NYPLAudiobookToolkit. Also, it is a singleton so it's better not to expose it to every framework.

@ErnestFan
Copy link
Contributor Author

@ettore The SPM unit tests failed because kSCNetworkReachabilityFlags is unavailable on mac-os.
I am thinking of updating the CI script, so the test is iOS-specific. However, swift test does not provide such option.
We will need to switch to xcodebuild in order to build and test, according to this article.
What's your thought on this?

@ettore
Copy link
Collaborator

ettore commented May 17, 2022

@ettore The SPM unit tests failed because kSCNetworkReachabilityFlags is unavailable on mac-os.
I am thinking of updating the CI script, so the test is iOS-specific. However, swift test does not provide such option.
We will need to switch to xcodebuild in order to build and test, according to this article.
What's your thought on this?

I would try to not build the Reachability source file for macOS, since it doesn’t really make sense for the Mac. If should be possible to do so in the Package.swift, or i suppose we could just do it with a #ifdef.

@ErnestFan
Copy link
Contributor Author

@ettore I added #if TARGET_OS_IPHONE and it now passes the unit test, thanks for the suggestion.

@ettore
Copy link
Collaborator

ettore commented May 17, 2022

This is fine for now since we don't have macOS apps. But I want to make a correction about what I said last night. Reachability does make sense on macOS and it does look supported according to the docs (since macOS 10.6) so yeah, I am not sure why it won't compile. I wonder if it was some kind of SPM caching issue......

@ErnestFan ErnestFan merged commit 92ba3ea into main May 17, 2022
@ErnestFan ErnestFan deleted the task/ios-393 branch May 17, 2022 17:27
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.

2 participants