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

chore: improve pattern matching experience for response types #90

Merged
merged 12 commits into from
Dec 12, 2023

Conversation

pgautier404
Copy link
Collaborator

No description provided.

@pgautier404 pgautier404 force-pushed the better-pattern-matching branch 2 times, most recently from 1a2eec5 to c831190 Compare December 9, 2023 00:58
@pgautier404 pgautier404 force-pushed the better-pattern-matching branch 3 times, most recently from b330d84 to 2d1ed6a Compare December 11, 2023 23:32
@pgautier404 pgautier404 force-pushed the better-pattern-matching branch from 2d1ed6a to 7778a94 Compare December 12, 2023 00:18
@pgautier404 pgautier404 marked this pull request as ready for review December 12, 2023 17:51
@pgautier404 pgautier404 requested a review from anitarua December 12, 2023 17:51
Pattern matching can be used to operate on the appropriate subtype.
```
switch response {
case let responseError as CacheDeleteError:
case let responseError as DeleteCacheError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

are the code examples in the docstrings going to have to change into enum case matching?

switch response {
case DeleteCacheError.error(let e):
   ...
case DeleteCacheError.success(let s):
   ...
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can pick up the task of updating the docstrings here while doing the other docstrings work though

Copy link
Collaborator

@anitarua anitarua left a comment

Choose a reason for hiding this comment

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

lgtm overall! hadn't realized we'd forgotten the delete cache item API but what you added looks good.
Had one comment about the docstrings, but I can pick up the task of reviewing docstrings again while working on that docstrings ticket

@pgautier404 pgautier404 merged commit 25247e5 into main Dec 12, 2023
4 checks passed
@pgautier404 pgautier404 deleted the better-pattern-matching branch December 12, 2023 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