-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
WIP: Add withLimit functionality #321
Conversation
Signed-off-by: Rohith-Raju <[email protected]>
Signed-off-by: Rohith-Raju <[email protected]>
Signed-off-by: Rohith-Raju <[email protected]>
Signed-off-by: Rohith-Raju <[email protected]>
@gavv Your thoughts on the implementation? I need some help with error messages. I'll reach out on chat 😄 |
Signed-off-by: Rohith-Raju <[email protected]>
Signed-off-by: Rohith-Raju <[email protected]>
Signed-off-by: Rohith-Raju <[email protected]>
☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts. |
reabase done
Signed-off-by: PiotrLewandowski323 <[email protected]>
Signed-off-by: Rohith-Raju <[email protected]>
Hi, The approach used in this patch is wrong. It's not allowed to read the whole body in bodyWrapper constructor - if you look at bodyWrapper's code, you'll see that it intentionally delays reading until Read/Close/GetBody is called first time. Also even if it was acceptable approach, it seems that it's implemented incorrectly, because you're just dropping everything you've read from body. We currently have another PR that modifies bodyWrapper: #342, which will conflict with this one. It makes sense to postpone this issue until #342 is merged, and re-implement it from scratch based on updated bodyWrapper. |
Initial commit that closes #271
Changes Made are
newBodyWrapperLimit
limit
field and introduced theWithLimit()
funcPending changes