Two new methods to access response headers#12
Two new methods to access response headers#12dudzio12 wants to merge 39 commits intoDaKaZ:masterfrom
Conversation
Fix bogus maybe-initialization of contenttype.
…nection is closed.
DaKaZ
left a comment
DaKaZ
left a comment
There was a problem hiding this comment.
Hello @dudzio12 - thank you for this PR and all the hard work you put into it. In general, I am incredibly thankful that this library is still being used and you took the time to update it! I candidly haven't developed on the esp8266 for the last 3 years so my maintenance has not been good. With all that said, I found a few area I think we should discuss/address before I could accept this PR. Take a look and let me know what you think. Thanks again!
| request += "\r\n\r\n"; | ||
| } | ||
|
|
||
| delay(0); |
There was a problem hiding this comment.
What is the purpose of this delay?
| while (sslClient.connected()) { | ||
| HTTP_DEBUG_PRINT("."); | ||
| // HTTP_DEBUG_PRINT("."); | ||
| delay(0); |
There was a problem hiding this comment.
Why add this delay? Probably should remove the comment on HTTP_DEBUG_PRINT as well - lets keep the original debug output consistent.
| HTTP_DEBUG_PRINT(","); | ||
|
|
||
| // HTTP_DEBUG_PRINT(","); | ||
| delay(0); |
There was a problem hiding this comment.
Why add this delays? Probably should remove the comment on HTTP_DEBUG_PRINT
|
|
||
| if (c == '\n') { | ||
| // you're starting a new line | ||
| // your starting a new line |
There was a problem hiding this comment.
Its silly, but the original grammar is correct: "you are" starting a new line
|
|
||
| delay(0); | ||
| char c = client.read(); | ||
| HTTP_DEBUG_PRINT(c); |
There was a problem hiding this comment.
I would rather not revert all this debugging.
| }else { | ||
| while (client.connected()) { | ||
| HTTP_DEBUG_PRINT("."); | ||
| while (client.connected() || client.available()) { |
There was a problem hiding this comment.
I don't think you want this extra || client.available() here. While it should never be true if connected() is false it incorrectly tells other developers that we should execute this block even if the client is not connected.
| return code; | ||
| } | ||
|
|
||
| String RestClient::getRawLastResponse() { |
There was a problem hiding this comment.
Perhaps a slightly better name would be getLastRequestRaw
| return rawLastResponse; | ||
| } | ||
|
|
||
| void RestClient::getResponseHeader(const char* key, String* value) { |
There was a problem hiding this comment.
Perhaps a slightly better name would be getLastResponseHeader
| String fullLengthPrefix = (String) key + ": "; | ||
| HTTP_DEBUG_PRINT(fullLengthPrefix); | ||
| int start = rawLastResponse.indexOf(fullLengthPrefix); | ||
| int end = rawLastResponse.indexOf("\n", start); |
There was a problem hiding this comment.
According to the spec https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html the end of the end of the headers should be \r\n\r\n
| contentType = "application/x-www-form-urlencoded"; // default | ||
| } | ||
| ssl = false; | ||
| contentType = "application/x-www-form-urlencoded"; |
There was a problem hiding this comment.
The consumer may still call setContentType so we should keep the conditional logic around these
if (!contentType) {
contentType = "application/x-www-form-urlencoded"; // default
}
4 participants
Hi,
Thanks for updating repository. Since that the original one is no longer working on newest libs - I prepared PR on your forked repo.
I've added 2 new methods, you can check it on updated readme.md.