carekit-apple / CareKit

CareKit is an open source software framework for creating apps that help people better understand and manage their health.
https://www.researchandcare.org
Other
2.41k stars 444 forks source link

Async/Await extensions for OCKStoreProtocol Constituents #617

Closed JWStaiert closed 3 years ago

JWStaiert commented 3 years ago

I have added async/await extensions to all constituent protocols of OCKStoreProtocol and fixed miscellaneous documentation typos in those protocols.

These changes compile cleanly with Xcode 13 beta 4 and do run on iOS 15 simulator. However I haven't tested every method, nor have I committed any of my unit tests. If anyone feels unit tests are needed, please comment.

erik-apple commented 3 years ago

I think it's fine that there aren't unit tests for each of these methods. They're just convenience wrappers around methods that are already thoroughly unit tested.

Perhaps in the future we could update the existing unit tests to use these new methods instead. That would get us a little bit better coverage without introducing any new tests, and we could remove the OCKStoreProtocol+Synchronous.swift file altogether!

JWStaiert commented 3 years ago

Changes made.

JWStaiert commented 3 years ago

Am I responsible for updating ".github/workflows/swift.yml"?

CI is failing because the build is using Xcode 12. Package.swift may also need updating.

erik-apple commented 3 years ago

Am I responsible for updating ".github/workflows/swift.yml"?

If it needs to be updated, it should be done as part of this PR. I can make the change for you if you allow administrators to make changes to your branch, or you can do it yourself.

What needs to be updated in Package.swift?

JWStaiert commented 3 years ago

I'll do it. And will verify if Package.swift needs and update (to the platform iOS version.)

cbaker6 commented 3 years ago

GitHub actions doesn’t support the Xcode beta publicly yet. To get the CI to pass as well as have CareKit build for anyone else who doesn’t have beta, the added code/files should be wrapped in:

#if swift(>=5.5)
@available(iOS 15.0, watchOS 9.0, *)
public extension OCKAnyReadOnlyCarePlanStore {
    …
}
#endif

Also, you should add watchOS since this Carekit works on watchOS

JWStaiert commented 3 years ago

Erik,

No rush, sometime after the weekend is great: when you have a moment would you comment on @cbaker6 's request?

cbaker6 commented 3 years ago

This is looking good!

I have one more request before we merge this work in. Could you open a new issue on GitHub to remove the #if swift(>=5.5) bits once Xcode 13 is supported by CI? Then, copy a link to that issue and paste it into the source code right above the places where we're using conditional compilation? That should help us remember why those compiler directives are there and remind us to remove them once it becomes possible to.

// Remove this once Xcode 13 is available on GitHub actions
// https://github.com/carekit-apple/carekit/issues/???
#if swift(>=5.5)

If you remove this, the builds after the removal won't work with iOS 13 and watchOS older than 9. From what I've seen, older Xcode versions will acknowledge #if swift(>=5.5), but will try to build @available(iOS 15.0, watchOS 9.0, *) even if those OS's are not available. If the goal is to only have CareKit for iOS 15+ then of course what I mentioned doesn't matter.

erik-apple commented 3 years ago

There is an important distinction here between Xcode 13 and iOS 15.

Once Xcode 13 is released, using @available(iOS 15.0, watchOS 9.0, *) will compile even for iOS 13 and 14.

This is not true for Xcode 12, because Xcode 12 doesn't know what iOS 15 and watchOS 9 are, so the compiler directives are required.

All that to say that CareKit will eventually require Xcode 13, but will still build for iOS 14.

cbaker6 commented 3 years ago

There is an important distinction here between Xcode 13 and iOS 15.

Once Xcode 13 is released, using @available(iOS 15.0, watchOS 9.0, *) will compile even for iOS 13 and 14.

This is not true for Xcode 12, because Xcode 12 doesn't know what iOS 15 and watchOS 9 are, so the compiler directives are required.

All that to say that CareKit will eventually require Xcode 13, but will still build for iOS 14.

Makes sense, thanks for the clarification!

erik-apple commented 3 years ago

Congrats on your first CareKit PR! I know a lot of people will appreciate this one!