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

Multifetch Adapter #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

mchapman
Copy link

No description provided.

Copy link
Owner

@jonsamwell jonsamwell left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for the pull request. It looks great however, I think the name of the adapter should be changed to indicate it only deals with sending GET requests; something like 'HttpGetMultiFetchAdapter'. Also a test verifying the de-serialization of the adapter would be good. This would be beneficial for someone reading code to figure out what the response structure should be. Also maybe add a quick explanation of the adapter and what server version it works with to help others understand if they can use it (as a class level comment). Once again many many thanks for the pull request!

@mchapman mchapman changed the title fix(build): Make bundle OS independent Multifetch Adapter Dec 29, 2017
@mchapman
Copy link
Author

mchapman commented Dec 29, 2017

Have done the rename and explanation. I have deferred the test until you have done one in your adapter - much easier to copy yours (you can see how incompetent I am by the accidental submission of this PR when I was just trying to submit a one line PR to sort out the build process).

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