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

Headers were not included on batch items. #107

Merged
merged 2 commits into from
Jun 6, 2020
Merged

Headers were not included on batch items. #107

merged 2 commits into from
Jun 6, 2020

Conversation

melgish
Copy link
Contributor

@melgish melgish commented Jun 4, 2020

I discovered that batch processing was not actually headers to the individual requests.

Updated getHeaders to correct the issue.

@janhommes
Copy link
Owner

lgtm. Just wondering if we should keep the inline comments. They help me to understand the PR, but I guess at some point we will wonder why we put them there.

@melgish
Copy link
Contributor Author

melgish commented Jun 4, 2020

Well that's up to you. I think having them explain 'why' a function is written one way vs another is useful. Keeps me from changing it back 2 days later :-) I also think anything that makes the code easier to understand encourages participation in the development.

@janhommes
Copy link
Owner

yeah, discussable. I guess when code changes often they are scared that the inline comments go out of sync. But as this code doesn't change that often I guess it is fine.

Will merge.

@janhommes janhommes merged commit 7f60418 into janhommes:master Jun 6, 2020
@melgish melgish deleted the bugfix/batch-headers branch June 8, 2020 12:23
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