Skip to content

Conversation

aaronflorey
Copy link
Contributor

No description provided.

@aaronflorey
Copy link
Contributor Author

once/if you accept my two PRs, i'll submit PRs for new adapters to use the new code, as well as a PR for the docs

Copy link
Contributor

@ksassnowski ksassnowski left a comment

Choose a reason for hiding this comment

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

Can you please also add tests for the new code? There should be tests to check that

  • Requests with set responses should not get sent
  • Requests with set responses still get passed through the middleware stack (i.e. they get passed to onResponseReceived)
  • withResponse returns a new request with the provided response set but does not alter the original request

You can look at the existing DownloaderTest and RequestTest tests for reference. Let me know if you need help implementing the tests.

@ksassnowski
Copy link
Contributor

i'll submit PRs for new adapters to use the new code

Can you clarify what you mean by adapters?

@aaronflorey
Copy link
Contributor Author

i'll submit PRs for new adapters to use the new code

Can you clarify what you mean by adapters?

Sorry, i meant the new classes like RedisQueue and CacheMiddleware.

Thanks for the feedback, sorry that you had to flag so many issues, i'll fix them soon!

@aaronflorey aaronflorey requested a review from ksassnowski March 27, 2023 23:52
Copy link
Contributor

@ksassnowski ksassnowski left a comment

Choose a reason for hiding this comment

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

I had a few more comments on the tests but looks good otherwise 👍 You can ignore the failing commit lint action for now as I will squash the commit anyway and can fix it then.

@aaronflorey aaronflorey force-pushed the response-middleware branch from e4c9373 to c452738 Compare March 28, 2023 06:06
@aaronflorey
Copy link
Contributor Author

Thank you for your patience!

@aaronflorey aaronflorey requested a review from ksassnowski March 28, 2023 06:07
@ksassnowski
Copy link
Contributor

Can you have look at the merge conflict? Looks good otherwise!

@ksassnowski
Copy link
Contributor

Thanks for the contribution!

Sorry, i meant the new classes like RedisQueue and CacheMiddleware.

Please hold off on this for a bit because I'm not sure yet if I want to include a caching middleware in the core library yet. It would be a useful third-party package, however.

PRs to the docs would be appreciated though 😊

@ksassnowski ksassnowski merged commit d8ae43e into roach-php:main Mar 28, 2023
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