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.

Sign up for GitHub

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

Jump to bottom

WIP: Add withLimit functionality #321

Closed
wants to merge 20 commits into from
Closed

WIP: Add withLimit functionality #321

wants to merge 20 commits into from

Conversation

Rohith-Raju
Copy link
Contributor

@Rohith-Raju Rohith-Raju commented Mar 2, 2023 •
edited

Initial commit that closes #271

Changes Made are

  • Added a new func newBodyWrapperLimit
  • Added a private limit field and introduced the WithLimit() func
  • Test cases for withLimit func

Pending changes

  • Formatting error messages and making them human-readable.
  • More unit tests
  • End-to-End tests

@Rohith-Raju
added withLimit
bc972cd 
Signed-off-by: Rohith-Raju <[email protected]>
@Rohith-Raju
cleanup
761c583 
Signed-off-by: Rohith-Raju <[email protected]>
@Rohith-Raju
merge changes
72d8910 
Signed-off-by: Rohith-Raju <[email protected]>
@Rohith-Raju
resloved merge conflicts
327c949 
Signed-off-by: Rohith-Raju <[email protected]>
@Rohith-Raju Rohith-Raju marked this pull request as draft March 2, 2023 14:37
@Rohith-Raju
Copy link
Contributor Author

@gavv Your thoughts on the implementation? I need some help with error messages. I'll reach out on chat 😄

@Rohith-Raju Rohith-Raju changed the title WIP: Add witlLimit functionality WIP: Add withLimit functionality Mar 3, 2023
@coveralls
Copy link
Collaborator

coveralls commented Mar 3, 2023 •
edited

Coverage Status

Coverage: 95.064% (-0.003%) from 95.067% when pulling 3706030 on Rohith-Raju:withLimit into fc83351 on gavv:master.

@Rohith-Raju Rohith-Raju marked this pull request as ready for review March 3, 2023 14:34
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Mar 3, 2023
@gavv
Copy link
Owner

gavv commented Mar 22, 2023

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.

@gavv gavv closed this Mar 22, 2023
@gavv gavv mentioned this pull request Mar 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs rebase Pull request has conflicts and should be rebased
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Request.WithLimit
6 participants
@Rohith-Raju @coveralls @gavv @nhAnik @gpjservais @PiotrLewandowski323