Skip to content
This repository was archived by the owner on Sep 26, 2018. It is now read-only.

Refactor deprecated Transport.CancelRequest method #12

Open
mihaitodor wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor deprecated Transport.CancelRequest method #12

mihaitodor wants to merge 1 commit into from

Conversation

@mihaitodor
Copy link

@mihaitodor mihaitodor commented May 12, 2017 •
edited
Loading

  • Use context.WithCancel to cancel the watcher http request
  • Refactor code to allow closing the request body in a sane way
  • Declare variables in the narrowest possible scope

Relevant discussions here and here.

@mihaitodor mihaitodor mentioned this pull request May 12, 2017
Merged
@mihaitodor mihaitodor force-pushed the refactor-deprecated-method branch 4 times, most recently from 43bfb19 to e8e5ae3 Compare May 19, 2017 11:10
@mihaitodor
Copy link
Author

mihaitodor commented May 19, 2017

@relistan @bparli Please have a look & let me know if you agree with this refactor

@bparli
Copy link

bparli commented May 19, 2017

We obviously need to use context here now, but I still don't see all the other changes as being necessary.

@mihaitodor
Copy link
Author

mihaitodor commented May 19, 2017 •
edited
Loading

I still don't see all the other changes as being necessary.

But I need to call the cancel() functor somewhere (the one returned by context.WithCancel). How should I do that in the current design? I'd like to follow the defer pattern, unless there is a good reason not to.

@mihaitodor
Refactor deprecated Transport.CancelRequest method
8a26f01 
- Use context.WithCancel to cancel the watcher http request
- Refactor code to allow closing the request body in a sane way
- Declare variables in the in the narrowest possible scope
@mihaitodor mihaitodor force-pushed the refactor-deprecated-method branch from e8e5ae3 to 8a26f01 Compare May 23, 2017 12:30
@mihaitodor mihaitodor force-pushed the master branch 2 times, most recently from e6c2040 to 7cfcede Compare August 24, 2017 11:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mihaitodor @bparli