Code cleanup #678
Merged
Code cleanup #678
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
http.NewRequest already does exactly what setRequestBody did. In fact, scrape and setRequestBody used to contain parts of http.NewRequest implementation. 119df7a implies that NewRequest is slower than assembling http.Request object manually, likely because it avoids URL reparsing. Indeed, this change brings some slowdown: name old time/op new time/op delta OnHTML-24 129µs ± 0% 131µs ± 1% +1.69% (p=0.029 n=4+4) OnXML-24 130µs ± 0% 132µs ± 1% +1.69% (p=0.029 n=4+4) OnResponse-24 93.4µs ± 0% 95.5µs ± 1% +2.30% (p=0.029 n=4+4) However, the benchmarks are somewhat unrealistic as the HTML page is extremely simple, the web server runs on localhost, and the data is discarded (rather than e.g. stored somewhere). In practice, impact of this is going to be neglible, and code complexity and code duplication is not really worth it.
streamToByte has an ugly type switch that tries to rewind the body so it could be read again. However, http.NewRequest already does something similar: when Body type allows, GetBody is populated so request can be retried. So just use it instead of trying to reimplement the body rewinding, and remove streamToByte function altogether.
The algorithm - specific hash function and what data is hashed - was duplicated in several locations. Move it to separate function.
asciimoo
reviewed
Mar 8, 2022
| // replace this with http.NewRequestWithContext | ||
| req = req.WithContext(c.Context) | ||
| setRequestBody(req, requestData) | ||
| if err := c.requestCheck(u, parsedURL, method, req.GetBody, depth, checkRevisit); err != nil { |
There was a problem hiding this comment.
Probably I'm missing something, but I can't find where the req.GetBody function is implemented. Pls help. =]
There was a problem hiding this comment.
GetBody is set by NewRequest: https://cs.opensource.google/go/go/+/refs/tags/go1.17.8:src/net/http/request.go;l=889-930;drc=refs%2Ftags%2Fgo1.17.8
This code fragment used to be copy-pasted in Colly (setRequestBody) and was removed in my previous commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are several commits, which are largely independent, but they touch the same code, so I thought it would be better to batch them together into single pull request instead of doing 2 extra rebases. Of course, if some of them are more questionable than others, I can split them to separate PRs. Please see the commit messages themselves for the description of what and why I attempted to do.