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

point to the same context after http.Request created #207

Closed
wants to merge 1 commit into from

Conversation

clippit
Copy link

@clippit clippit commented Jan 4, 2019

This pull request adds checks related to context methods. When http.Request is created, context between resty.Request and http.Request stays the same. This can avoid mess when working with preReqHook.

See #206

@codecov
Copy link

codecov bot commented Jan 4, 2019

Codecov Report

Merging #207 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   96.15%   96.17%   +0.01%     
==========================================
  Files           9        9              
  Lines        1119     1123       +4     
==========================================
+ Hits         1076     1080       +4     
  Misses         23       23              
  Partials       20       20
Impacted Files Coverage Δ
request17.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63ac674...708c175. Read the comment docs.

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@clippit Thanks for the PR but I'm unable to merge this. Please refer to my inline comment.

@@ -67,6 +70,9 @@ func (r *Request) Context() context.Context {
// documentation.
func (r *Request) SetContext(ctx context.Context) *Request {
r.ctx = ctx
if r.RawRequest != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@clippit I have intentionally did not make these changes. Because by resty design PreRequestHook is only meant to modify RawRequest not resty Request` (refer to #62 ). Any changes made in resty Request instance will not affect the request.

@@ -55,6 +55,9 @@ type Request struct {
// Context method returns the Context if its already set in request
// otherwise it creates new one using `context.Background()`.
func (r *Request) Context() context.Context {
if r.RawRequest != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@clippit I have intentionally did not make these changes. Because by resty design PreRequestHook is only meant to modify RawRequest not resty Request` (refer to #62). Any changes made in resty Request instance will not affect the request.

@clippit
Copy link
Author

clippit commented Jan 6, 2019

I understand.
Only expose RawRequest in PreRequestHook may be a better option since whatever you want to get from Request can also be fetched from RawRequest, which also makes resty more opinionated about this hook. But this would be an API break change, perhaps you can consider it in next major version.

@jeevatkm
Copy link
Member

jeevatkm commented Jan 6, 2019

@clippit Thank you. Yes, I have realized when I was reviewing your PR. I will surely change hook signature in upcoming v2. Just added as action item in #167

@clippit clippit deleted the context branch January 6, 2019 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants