-
-
Notifications
You must be signed in to change notification settings - Fork 772
Includes Cookies in Debug Request Log #261
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
Conversation
Minor formatting improvements. Fixes #260.
Codecov Report
@@ Coverage Diff @@
## v1.x #261 +/- ##
==========================================
- Coverage 96.17% 95.93% -0.24%
==========================================
Files 9 9
Lines 1123 1131 +8
==========================================
+ Hits 1080 1085 +5
- Misses 23 25 +2
- Partials 20 21 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewpoer Thank you for the PR to improve Resty debug logging.
Could you please retarget your PR to master branch the also add a test case?
| fmt.Sprintf("%s %s %s\n", r.Method, rr.URL.RequestURI(), rr.Proto) + | ||
| fmt.Sprintf("HOST: %s\n", rr.URL.Host) + | ||
| fmt.Sprintf("HEADERS:\n%s\n", composeHeaders(rl.Header)) + | ||
| fmt.Sprintf("COOKIES:\n%s\n", composeCookies(c.GetClient().Jar, *rr.URL)) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewpoer Just remembered one thing. This method focuses on Cookies from Jar and creates a section called COOKIES. Since cookies get separate section in the debug log. I think its better to read request header cookies added by the user into the cookies section finally remove the request header Cookie from headers section. Let me know your thoughts.
|
@matthewpoer If it's okay with you. Shall I close this PR and recreate changes from this PR as a branch? |
|
@matthewpoer I'm planning to make the release of v2.1.0. So, I'm going to take over your PR and apply the required change for the release. |
|
This PR has been merged via local merge targeting branch |
Minor formatting improvements. Fixes #260.