Skip to content

Conversation

crazytonyli
Copy link
Contributor

Note

This PR is built on top of #136

This PR also adds a SafeRequestExecutor type to avoid accidentally throwing random error instances into Rust code, which would cause crash (discussed in #136 (comment)).

@crazytonyli crazytonyli requested a review from jkmassel June 5, 2024 07:04
Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left one suggestion (probably for later), and pushed SwiftLint fixes

self.fetchUsersTask = Task { @MainActor in
do {
users = try await api.users.forViewing.list()
users = try await api.users.listWithViewContext(params: nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily for this PR, but are we able to make it so we can just call this like try await api.users.listWithViewContext() and provide params if needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. That's implemented in the old API design where we had generic types to check if the params argument is Optional. I'll look into it later, potentially together with providing a better API regarding handling the new RequestExecutionError type.

Copy link
Contributor Author

@crazytonyli crazytonyli Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, we can just implement a few listWith[View|Edit|Embed]Context() functions to Swift, if we can't have a general solution in short term.

Base automatically changed from request-executor to trunk June 5, 2024 17:09
@jkmassel jkmassel force-pushed the request-executor-swift-package branch from c87af46 to 5fc35fe Compare June 5, 2024 17:28
@jkmassel
Copy link
Contributor

jkmassel commented Jun 5, 2024

I also rebased this on trunk because #136 landed

@crazytonyli crazytonyli enabled auto-merge (squash) June 5, 2024 21:22
@crazytonyli crazytonyli merged commit 7ce62e5 into trunk Jun 5, 2024
@crazytonyli crazytonyli deleted the request-executor-swift-package branch June 5, 2024 21:41
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