Skip to content
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

Application service #84

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Application service #84

merged 3 commits into from
Sep 7, 2023

Conversation

djc
Copy link
Owner

@djc djc commented Sep 7, 2023

@valkum this is more like what I had in mind -- this seems like the simplest formulation that makes it easy to use an Application to build a hyper Server. What do you think?

Alternative to #82.

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
mendes/src/application.rs 100.00%
mendes/src/hyper.rs 100.00%

📢 Thoughts on this report? Let us know!.

@valkum
Copy link
Contributor

valkum commented Sep 7, 2023

I assume you can't call Into::into because Rust can't infer the type to convert into?
What's the benefit of

hyperServer::bind()
  .serve(ApplicationService::from(App::default()))
  .await

vs

hyperServer::bind()
  .serve(App::default().into_hyper_service())
  .await

We won't need to make ApplicationService completely pub (pub in private module should be enough), keeping the API interface cleaner.

@djc
Copy link
Owner Author

djc commented Sep 7, 2023

I assume you can't call Into::into because Rust can't infer the type to convert into? What's the benefit of

hyperServer::bind()
  .serve(ApplicationService::from(App::default()))
  .await

vs

hyperServer::bind()
  .serve(App::default().into_hyper_service())
  .await

Changed it to add a provided method on Application.

We won't need to make ApplicationService completely pub (pub in private module should be enough), keeping the API interface cleaner.

IMO that's a minimal difference and as a user I have found types that are pub in private module very frustrating (a type in the docs that you can't click to get more info). I'd rather expose the public type, which is pretty opaque anyway.

@valkum
Copy link
Contributor

valkum commented Sep 7, 2023

Looks good.

@djc djc merged commit 0d7d1b2 into main Sep 7, 2023
11 checks passed
@delete-merged-branch delete-merged-branch bot deleted the application-service branch September 7, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants