Skip to content
This repository was archived by the owner on Jun 21, 2020. It is now read-only.

Write docs #19

Merged
merged 18 commits into from
Apr 25, 2019
Merged

Write docs #19

merged 18 commits into from
Apr 25, 2019

Conversation

gruberb
Copy link

@gruberb gruberb commented Apr 19, 2019

I added a first example. I came up with a lot of questions which hopefully can be answered by a more experienced http-service maintainer:

  • Do we separate the hyper-http-service examples from the http-service ones? If so, we can't show how to actually start a server but just how to compose one?
  • When building an example implementation of hyper-http-service, we need to re-create our http-server again?
  • I would need additional help with these two topics:
    • Text for the README: What is the overall goal of http-service?
    • Additional use cases: I mapped out one in the first example. I would need a bit of input of how other use cases might look like

Motivation and Context

Making http-service more presentable and accessible (issue)

Checklist:

  • Finish up first example
  • Find more use cases and implement examples for them
  • Update README with basic FAQ and introduction
  • Add doc tests

After working on this for a while, I would even suggest to merge hyper-http-service and http-service? Since I can't see the use case for building a http-service and then starting it differently? The user has to be aware to include http-service-hyper in their Cargo.toml, which is not really obvious.

@gruberb gruberb requested a review from yoshuawuyts April 19, 2019 05:22
Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Looks great!

Could we maybe also add the example to the README.md and lib.rs files to make it a bit clearer how this works for people that just stumble upon it?

@gruberb
Copy link
Author

gruberb commented Apr 21, 2019

Done! I added the main() in the example in the lib.rs, although we need http-hyper-service there to run it. Is that ok?

I also don't quite know what's the best way is to create doc tests. Could you point me in the right direciton?

The README is now "complete", I copied your style from the runtime crate :)

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Looking great!

@yoshuawuyts
Copy link
Member

@gruberb looks like cargo fmt checks are failing; could you run cargo fmt and try again?

@gruberb
Copy link
Author

gruberb commented Apr 23, 2019

@yoshuawuyts I am trying to figure out how to use a workspace member in my simple_response.rs. Do I need to use #[cfg] here or how can I access http-service-hyper inside my example?

@yoshuawuyts
Copy link
Member

@gruberb you need to specify it as a dependency as a path to the local file. And in order to be able to publish your crate you also need to give it a fallback version. Example of how to do both here: https://github.com/rustasync/runtime/blob/master/Cargo.toml#L27

@gruberb
Copy link
Author

gruberb commented Apr 23, 2019

I struggle to figure out what changed since the latest commit: a4870c1

Since then, my example doesn't compile anymore:

expected an `Fn<(http::request::Request<http_service::Body>,)>` closure, found `Server`

I think we should still be able to pass a simple Server which implements HttpService to the run method.

@gruberb
Copy link
Author

gruberb commented Apr 24, 2019

@yoshuawuyts After lots of experimenting with the doc tests, the PR is ready in my view. I couldn't make the example in the docs work (https://github.com/rustasync/http-service/pull/19/files#diff-b4aea3e418ccdb71239b96952d9cddb6R5). So I ignored it. It's the exact same code as in the simple_response.rs, so I don't exactly know why cargo test would fail on this point.

The backtrace wasn't helpful either.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@yoshuawuyts yoshuawuyts merged commit 1f30e91 into http-rs:master Apr 25, 2019
@gruberb gruberb deleted the write-docs branch April 26, 2019 04:56
@gruberb gruberb mentioned this pull request Apr 26, 2019
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants