-
Notifications
You must be signed in to change notification settings - Fork 3
Implement users endpoints in Swift #62
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
Conversation
import Foundation | ||
import PackageDescription | ||
|
||
let isCI = ProcessInfo.processInfo.environment["BUILDKITE"] == "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most CI systems (including Buildkite) define CI=true
I think we could probably just use that?
@@ -0,0 +1,19 @@ | |||
// Expose necessary Rust APIs as public API to the Swift package's consumers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a super interesting idea.
I'd originally thought we'd do @_exported import
but this is pretty tidy and not a _ton_of overhead 🤔
Can a typealias
conform to a protocol?
I'd imagine each export type will need ID
, and Context
, so maybe that'd be valuable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a typealias conform to a protocol?
I think so.
Type aliases don’t create new types; they simply allow a name to refer to an existing type. Link
|
||
public extension SparseUser { | ||
typealias ID = UserId | ||
typealias View = UserWithViewContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the single-word types here – I think whatever the type name is here it probably needs Context
in it?
|
||
extension WordPressAPI { | ||
public var users: Namespace<SparseUser> { | ||
.init(api: self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Organizing things with namespaces seems kinda nice, but I wonder about the additional overhead here with generics (and if it's worthwhile/avoidable?)
You could have:
UserNamespace
as a struct and define all of these methods without needing quite as much bookkeeping by the compiler?
Also a nit – I wonder if it'd be possible to encourage the compiler to inline this allocation for us – the underlying object isn't really needed at runtime?
WDYT?
|
||
} | ||
|
||
extension Namespace where T == SparseUser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this separation of "common" methods vs "object-specific" methods
@@ -0,0 +1,172 @@ | |||
#if os(macOS) || os(Linux) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the kind of end-to-end test we want, but I think we probably want to push it down into the Rust layer?
WDYT?
a935e3c
to
86ef464
Compare
Closing in favor of #67 |
Note
This PR builds on top of #58. It's ready for review, but I won't merge it until #58 is merged.
Other than implementing CRUD functions on users endpoints, this PR introduces a few new things that will be applied to all endpoints:
WordPressAPI
, they are at a layer deep:WordPressAPI.users
. That means developers writewordPressApi.users.get(id: 1)
, instead ofwordPressApi.getUser(id: 1)
. This is a simple syntax suger to make it easier to work with code completion.Exports.swift
to pick and choose types in the Rust code to be part of thewordpress-api
module (which probably should be renamed, but that's for later).One thing missing is passing
context
argument to the reading APIs, which is still WIP.