Extract models into a separate Core module #437
Conversation
| ), | ||
| .testTarget( | ||
| name: "MeiliSearchUnitTests", | ||
| dependencies: ["MeiliSearch"] |
There was a problem hiding this comment.
nit: where we reference target names, let's be consistent and use the static variable
| dependencies: ["MeiliSearch"] | |
| dependencies: [.target(MeiliSearch)] |
| dependencies.append(.package(url: "https://github.com/swift-server/async-http-client.git", from: "1.19.0")) | ||
| targets.append( | ||
| .target( | ||
| name: MeiliSearchNIO, |
There was a problem hiding this comment.
major: this PR will currently fail to build as-is due to this target path not existing. shall we delete this entire Linux section until you contribute the NIO integration in your next PR?
| ) | ||
| ] | ||
|
|
||
| #if os(Linux) || USE_NIO |
There was a problem hiding this comment.
comment: I like the use of USE_NIO here as it'll allow macOS clients (for working on Vapor projects locally) to build the NIO integration. When we add the NIO integration (presumably the next PR), let's document how to use this.
| private static let current = "0.15.0" | ||
| public static let current = "0.15.0" |
There was a problem hiding this comment.
question: why has the access level of this variable changed? where else is it used?
| private static let current = "0.15.0" | ||
| public static let current = "0.15.0" |
There was a problem hiding this comment.
minor: let's make sure we update the documentation where this line of code is referenced (the path to this file)
| private static let current = "0.15.0" | ||
| public static let current = "0.15.0" |
There was a problem hiding this comment.
minor: let's make sure we update the Podspec where this line of code is referenced (the path to this file)
| @@ -1,4 +1,5 @@ | |||
| @testable import MeiliSearch | |||
| @testable import MeiliSearchCore | |||
There was a problem hiding this comment.
question: do we require @testable for the core target? 99% of models within are public (applicable to all tests)
| ] | ||
| ), | ||
| .target( | ||
| name: MeiliSearchCore, |
There was a problem hiding this comment.
comment: this new target should not break the Podspec as it does a global search for all Swift files within the Sources directory, however when we do add the NIO target it will fail to compile due to the lack of the dependency.
AHC (or NIO) is not available officially on CocoaPods, so this will be a SPM-only feature. I'm okay with that but we must document it and ensure that CocoaPods does continue to work (even if only using Foundation).
|
@Sherlouk Thanks for the review! I wasn't expecting it yet as this is still a draft but happy to address the comments I hadn't planned to address. |
No worries, thought I may as well share some guiding thoughts. Looks like great progress towards the shared goal 👍 |
|
Hey @kaishin - just wondering how you're feeling about this? |
|
@Sherlouk The project where I used MeiliSearch got deprioritized during spring so I couldn't get myself to dedicate time to finish this, but that's gonna be changing this month so perhaps let's timebox this to August? If I don't get it done this month I am unlikely to finish it afterwards. |
This PR is the first step of addressing #434 .