Updated HTTPServerRequest docs #258
Conversation
Codecov Report
@@ Coverage Diff @@
## master #258 +/- ##
==========================================
- Coverage 74.81% 74.74% -0.08%
==========================================
Files 31 31
Lines 4181 4181
==========================================
- Hits 3128 3125 -3
- Misses 1053 1056 +3
Continue to review full report at Codecov.
|
| /** | ||
| This class implements the `ServerRequest` protocol for incoming sockets that are communicating via the HTTP protocol. Data and Strings can be read in. | ||
|
|
||
| ### Usage Example: ### |
There was a problem hiding this comment.
We should have something here to describe this example even if it's just something like
"In this example, we read the request data from the body of request and return the length of the data in bytes in the response object."
| /** | ||
| HTTP Status code if this message is a response | ||
|
|
||
| ### Usage Example: ### |
There was a problem hiding this comment.
We should remove this example as this method is marked as deprecated, so we don't want to encourage users to use it.
| ````swift | ||
| response!.httpStatusCode | ||
| ```` | ||
| */ | ||
| @available(*, deprecated, message: |
There was a problem hiding this comment.
jazzy doesn't support generating documentation for @available (realm/jazzy#843)
Therefore, as the current message is not very readable in the API docs can we make this a jazzy note, or does this need to be marked @deprecated to display as such within Xcode? Regardless please can we change the text to:
"This method does not work on ServerRequest objects, it was incorrectly inherited from a super class."
| public class HTTPServerRequest: ServerRequest { | ||
|
|
||
| /// HTTP Status code if this message is a response | ||
| /** | ||
| HTTP Status code if this message is a response |
There was a problem hiding this comment.
How can this be a response object when we're inside ServerRequest?
There was a problem hiding this comment.
Please add a comma and a full stop -> "HTTP Status code, if this message is a response."
| @available(*, deprecated, message: | ||
| "This method never worked on Server Requests and was inherited incorrectly from a super class") | ||
| public private(set) var httpStatusCode: HTTPStatusCode = .unknown | ||
|
|
||
| /// Client connection socket | ||
| private let socket: Socket | ||
|
|
||
| /// Server IP address pulled from socket. | ||
| /** | ||
| Server IP address pulled from socket. |
There was a problem hiding this comment.
Suggested reword -> "The server IP address from the socket."
|
|
||
| ### Usage Example: ### | ||
| ````swift | ||
| request.remoteAddress |
There was a problem hiding this comment.
We should at least assign this to a variable or constant. We could also print out an example, something like that below (but the second piece is optional)
let remoteAddress = request.remoteAddress
print(remoteAddress) // Prints xx.xx.xx.xx
(where xx.xx.xx.xx is an example real IP address)
| public var remoteAddress: String { | ||
| return socket.remoteHostname | ||
| } | ||
|
|
||
| /// HTTP Method of the request. | ||
| /** | ||
| HTTP Method of the request. |
There was a problem hiding this comment.
The HTTP method of the request.
(Method needs to be lowercase).
|
|
||
| ### Usage Example: ### | ||
| ````swift | ||
| request.method.lowercased() |
There was a problem hiding this comment.
Again assign to a constant or variable and ideally print out an example.
| public var method: String { return httpParser?.method ?? ""} | ||
|
|
||
| /// Major version of HTTP of the request | ||
| /** | ||
| Major version of HTTP of the request |
There was a problem hiding this comment.
"The HTTP major version of the request."
| public var httpVersionMajor: UInt16? { return httpParser?.httpVersionMajor ?? 0} | ||
|
|
||
| /// Minor version of HTTP of the request | ||
| /** | ||
| Minor version of HTTP of the request |
There was a problem hiding this comment.
"The HTTP minor version of the request."
| public var httpVersionMinor: UInt16? { return httpParser?.httpVersionMinor ?? 0} | ||
|
|
||
| /// Set of HTTP headers of the request. | ||
| /** | ||
| Set of HTTP headers of the request. |
There was a problem hiding this comment.
The word set is confusing in this description. Are we reading the headers in the request, or can we set them here too?
|
|
||
| ### Usage Example: ### | ||
| ````swift | ||
| let protocols = request.headers["Upgrade"] |
There was a problem hiding this comment.
Can we have a more obvious example than upgrade?
|
|
||
| ### Usage Example: ### | ||
| ````swift | ||
| public var signature: Socket.Signature? { return socket.signature } |
There was a problem hiding this comment.
This is a strange choice for the usage example.
| public var headers: HeadersContainer { return httpParser?.headers ?? HeadersContainer() } | ||
|
|
||
| /// socket signature of the request. | ||
| /** | ||
| socket signature of the request. |
There was a problem hiding this comment.
"The socket signature of the request."
| /// URLComponents has a memory leak on linux as of swift 3.0.1. Use 'urlURL' instead | ||
| /** | ||
| The URL from the request as URLComponents | ||
| URLComponents has a memory leak on linux as of swift 3.0.1. Use 'urlURL' instead |
There was a problem hiding this comment.
The first sentence needs a full stop at the end (as does the last)
swift -> Swift
We should remove the usage example if this is now deprecated.
As in the previous example above please try using a note rather than @available as this is not supported by jazzy
| /// This contains just the path and query parameters starting with '/' | ||
| /// Use 'urlURL' for the full URL | ||
| /** | ||
| The URL from the request in string form |
There was a problem hiding this comment.
Needs a full stop at the end and a comma after the word request.
You need to show an example full URL and then a urlString with just the path and query parameters to make this clearer.
| ````swift | ||
| print(request.urlString) | ||
| ```` | ||
| */ | ||
| @available(*, deprecated, message: |
There was a problem hiding this comment.
Why is this deprecated?
| /// This contains just the path and query parameters starting with '/' | ||
| /// Use 'urlURL' for the full URL | ||
| /** | ||
| The URL from the request in UTF-8 form |
There was a problem hiding this comment.
Needs full stops a the end of the sentences and a clear example.
| - Returns: the number of bytes read. | ||
|
|
||
| ### Usage Example: ### | ||
| ````swift |
There was a problem hiding this comment.
All three of the read() examples are very short with no explanation.
The parameters/throws/returns definitions all need to start with capital letters.
|
Approved, despite comments not being addressed. I may address the comments at a later date when I get chance, as they would be nice to have. |
Updated HTTPServerRequest docs with new code examples and improvements