[go: up one dir, main page]
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

Refactor httplib: Remove debug code to a filter #4440

Closed
flycash opened this issue Jan 13, 2021 · 6 comments
Closed

Refactor httplib: Remove debug code to a filter #4440

flycash opened this issue Jan 13, 2021 · 6 comments
Assignees
@SpikeWong
Labels
good first issue help-wanted We need someone help to resolve this issue inactive-issue

Comments

@flycash
Copy link
Collaborator
flycash commented Jan 13, 2021

The httplib.BeegoHTTPSettings has field ShowDebug bool
It only be used in method doRequest:

	if b.setting.ShowDebug {
		dump, err := httputil.DumpRequest(b.req, b.setting.DumpBody)
		if err != nil {
			log.Println(err.Error())
		}
		b.dump = dump
	}

But I don't see any code use b.dump. So I think we could remove b.dump and this part.

And then we provide a filter named LogFilter. It's default filter and its reponsibility is to log request and response. And if the request.ShowDebug = true, it logs request and response:

log.Debug(request)
next()
log.Debug(response)
@flycash flycash mentioned this issue Jan 13, 2021
Closed
@flycash flycash added good first issue help-wanted We need someone help to resolve this issue labels Jan 13, 2021
@SpikeWong
Copy link
Contributor
SpikeWong commented Jan 18, 2021 •
edited
Loading

@flycash If you do not hurry, I want to complete this task. Because I am not familiar with the source code of beego project, so I need a few days to get familiar with it. If all goes ok, I will submit a pr next week. Do you think this is ok?

@SpikeWong
Copy link
Contributor

@flycash DumpRequest() uses b.dump, so I think we can not remove it directly?

https://github.com/beego/beego/blob/develop/client/httplib/httplib.go#L184

@SpikeWong
Copy link
Contributor

I found that there is no place to call DumpRequest(). What scenarios will call this method at present? If not, consider deleting DumpRequest() and b.dump

@flycash
Copy link
Collaborator Author
flycash commented Jan 18, 2021

Thank you @anoymouscoder . This is not an urgent issue, and I assigned it to you.

I found that there is no place to call DumpRequest(). What scenarios will call this method at present? If not, consider deleting DumpRequest() and b.dump

I think we could remove both DumpBody bool, b.dump, method DumpRequest and DumpBody because users are able to call GetRequest to get original http.Request and then dump request by themselves.

The scenario is that users want to log request and response when they use httplib. In the future, they could use this LogFilter to log request.

@SpikeWong
Copy link
Contributor

@flycash Thank you for your detailed instrutions, I will start now!

@SpikeWong SpikeWong mentioned this issue Jan 19, 2021
Merged
flycash added a commit that referenced this issue Feb 2, 2021
@flycash
Merge pull request #4445 from anoymouscoder/refactor-filter
a8bfd40 
Refactor httplib: Move debug code to a filter #4440
@github-actions
Copy link

This issue is inactive for a long time.

@flycash flycash closed this as completed Mar 21, 2021
@flycash flycash mentioned this issue Apr 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SpikeWong SpikeWong

Labels
good first issue help-wanted We need someone help to resolve this issue inactive-issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@flycash @SpikeWong