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

[92] progress without error #117

Merged
merged 8 commits into from
Feb 13, 2023
Merged

[92] progress without error #117

merged 8 commits into from
Feb 13, 2023

Conversation

amyoulton
Copy link
Contributor

@amyoulton amyoulton commented Jan 23, 2023

This PR adds in the ability for a request to have a postProcessing method. To implement it, I have done the following:

  1. Added postProcess method to Netable request, that by default returns the FinalResource. This will now be the final method run through every request.
  2. If the user adds a post process method to the request, something can be run before the FinalResource is returned.
  3. I have added in a simple example to the Example project, where we have a Data Manager that stores a user. The GetUserRequest implements the postProcess method, where we send the user to the shared user object in the DataManager before returning the finalResource.
  4. In Homeview, I added a print method to showcase this working.

Note: I also removed the target here for the old example. I think that was also done in the sendable ticket which hasn't been merged, but if not it's done here now! Whichever ticket is merged first, I will rebase the other, so it shouldn't matter.

@amyoulton amyoulton force-pushed the aeo/92-post-processing-hook branch from cd00a64 to 2526949 Compare February 6, 2023 18:43
@amyoulton amyoulton marked this pull request as ready for review February 6, 2023 18:46
Copy link
Collaborator

@brendanlensink brendanlensink left a comment

Choose a reason for hiding this comment

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

A couple suggestions/comments to think about

Also we should probably bump the library version since we're adding new functionality ✨

Netable/Netable/Netable.swift Outdated Show resolved Hide resolved
Netable/Netable/Netable.swift Outdated Show resolved Hide resolved
Netable/Netable/Request.swift Show resolved Hide resolved
@amyoulton amyoulton assigned brendanlensink and unassigned amyoulton Feb 8, 2023
Copy link
Collaborator

@brendanlensink brendanlensink 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, before merging don't forget to add a blurb to the README explaining intended use 🚀

@amyoulton amyoulton force-pushed the aeo/92-post-processing-hook branch from 6d67d6a to 83cbe7d Compare February 9, 2023 18:30
@amyoulton amyoulton merged commit 920c0e9 into main Feb 13, 2023
@amyoulton
Copy link
Contributor Author

@brendanlensink this is merged! You said you were going to do the tag and release stuff so just nudging about that! 👀

@amyoulton amyoulton deleted the aeo/92-post-processing-hook branch February 13, 2023 22:20
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.

3 participants