Skip to content

Small README fixes #26

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

Merged
merged 2 commits into from
May 8, 2019
Merged

Small README fixes #26

merged 2 commits into from
May 8, 2019

Conversation

Yasumoto
Copy link
Contributor

@Yasumoto Yasumoto commented May 2, 2019

Including an update to fixup Timeout and Request after the refactor in #10.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@artemredkin artemredkin requested review from artemredkin and tomerd May 2, 2019 19:16
@artemredkin artemredkin added the kind/enhancement Improvements to existing feature. label May 2, 2019
README.md Outdated
@@ -49,8 +49,8 @@ httpClient.get(url: "https://swift.org").whenComplete { result in
}
```

It is important to close client instance after use to cleanly shutdown underlying NIO ```EventLoopGroup```:
```
It is important to close the client instance, either in a ```defer``` statement or class' ```deinit``` after use to cleanly shutdown underlying NIO ```EventLoopGroup```:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean shutdown worth a bit more explanation and examples. i worry folks will read this as they are suppose to call httpClient.syncShutdown() on deinit

//cc @weissi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @tomerd . Also please don't recommend or mention to use a class' deinit, that's poor style and will lead to really terrible bugs. People will still do it but at least we didn't recommend it :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😬great save!

README.md Outdated
@@ -61,7 +61,7 @@ In this case shutdown of the client is not neccecary.

## Usage guide

Most common HTTP methods are supported out of the box. In case you need to have more control over the method, or you want to add headers or body, use ```HTTPRequest``` struct:
Most common HTTP methods are supported out of the box. In case you need to have more control over the method, or you want to add headers or body, use the ```HTTPRequest``` struct:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single word quoting should be done with ` and not ``` because ``` opens a code-block which in some renderers gets its own line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I copied the style found elsewhere in this doc, I'll fixup 🚫

@Yasumoto
Copy link
Contributor Author

Yasumoto commented May 8, 2019

Heya folks! Friendly nudge to get this another review 🙏

@weissi weissi merged commit e0c1691 into swift-server:master May 8, 2019
@Yasumoto Yasumoto deleted the readme branch May 8, 2019 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants