-
Notifications
You must be signed in to change notification settings - Fork 346
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
Misc doc changes #446
Misc doc changes #446
Conversation
50797e3
to
63a4cd5
Compare
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.
❤️
I added some feedback.
And general comments
- I don't think adding a line break for long lines add significant values...
- Probably better to rephrase the headline of moduledoc to what it does, starting with third-person singular verbs for consistency (within tesla, and it's common in Elixir itself) - I added one example for a middleware. It is really useful when seeing a list of modules. e.g.
Use ...
=>Uses ...
README.md
Outdated
[ | ||
{:tesla, "~> 1.4.0"}, | ||
{:jason, ">= 1.0.0"}, # optional, required by JSON middleware | ||
{:hackney, "~> 1.10"} # or :gun etc. |
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.
I think it would be better to say when hackney is needed - instead of or :gun etc.
.
{:hackney, "~> 1.10"} # when using hackney adapter
README.md
Outdated
@@ -228,7 +229,7 @@ def new(...) do | |||
end | |||
``` | |||
|
|||
Passing directly to `get`/`post`/etc. | |||
Passing directly to `get` or etc. |
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.
or etc.
is incorrect, since etc.
= Et cetera
= "and other similar things"
https://en.wikipedia.org/wiki/Et_cetera
What about this?
Passing directly to request functions such as
Tesla.get/3
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.
Since the examples after the statement have two different function calls, I've updated to:
232 Passing directly to request functions such as `MyClient.get/3` or `Tesla.get/3`.
233
234 ```elixir
235 MyClient.get("/", opts: [adapter: [recv_timeout: 30_000]])
236 Tesla.get(client, "/", opts: [adapter: [recv_timeout: 30_000]])
237 ```
README.md
Outdated
@@ -382,7 +382,7 @@ defmodule Tesla.Middleware.SomeMiddleware do | |||
Longer description, including e.g. additional dependencies. | |||
|
|||
|
|||
### Example usage | |||
### Example |
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.
As Elixir itself uses Examples
- probably we can use that as well?
lib/tesla/middleware/compression.ex
Outdated
@@ -2,9 +2,9 @@ defmodule Tesla.Middleware.Compression do | |||
@moduledoc """ | |||
Compress requests and decompress responses. | |||
|
|||
Supports "gzip" and "deflate" encodings using erlang's built-in `:zlib` module. | |||
Supports `"gzip"` and `"deflate"` encodings using Erlang's built-in `:zlib` module. |
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.
Here, we refer gzip and deflate as the name of compression format - so don't need to wrap with ```.
I'm not sure what's the official or canonical name of the format. RFC uses "GZIP" and "DEFLATE" though
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.
Reverted to "gzip" and "deflate" instead.
lib/tesla/middleware/keep_request.ex
Outdated
@@ -1,6 +1,6 @@ | |||
defmodule Tesla.Middleware.KeepRequest do | |||
@moduledoc """ | |||
Store request body & headers into opts. | |||
Store request `body` and `headers` into opts. |
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.
I think "body" and "headers" here are referring the HTTP terms, not fields in the struct. However, opts
is a field.
What about this?
Keeps request body and headers in
opts
.
lib/tesla/middleware/logger.ex
Outdated
@@ -40,9 +40,11 @@ defmodule Tesla.Middleware.Logger do | |||
@moduledoc """ | |||
Log requests using Elixir's Logger. | |||
|
|||
With the default settings it logs request method, url, response status and time taken in milliseconds. | |||
With the default settings it logs request method, url, response status and |
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.
url => URL, if we want to make it consistent with other parts
@@ -1,9 +1,9 @@ | |||
defmodule Tesla.Middleware.MethodOverride do | |||
@moduledoc """ | |||
Middleware that adds X-Http-Method-Override header with original request | |||
Middleware that adds `X-Http-Method-Override` header with original request |
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.
I think it would be better to downcase the example as in the code.
Suggestion
Changes the request method to POST and adds
x-http-method-override
header with the original request method
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.
Based on https://opensocial.github.io/spec/2.5.1/Core-API-Server.xml#rfc.section.2.1.1.1 via https://en.wikipedia.org/wiki/List_of_HTTP_header_fields,
X-HTTP-Method-Override
seems to be the right name with case.
List of changes: - Bump license year - Use common source url - Fix typos - Fix markdown - Add release changelog to readme - Add hexdocs badge - Use and set ex_doc to latest version - Update gitignore
63a4cd5
to
301aade
Compare
Rebased and merged in 65d178a Thanks! |
List of changes: