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

Tide feature and improved example #94

Merged
merged 9 commits into from Aug 7, 2020
Merged

Tide feature and improved example #94

merged 9 commits into from Aug 7, 2020

Conversation

kaj
Copy link
Owner

@kaj kaj commented Jul 30, 2020

Provide a tide example similar to the other framework examples, with static files and error handling.

Provide a tide example similar to the other framework examples, with
static files and error handling.
@kaj kaj mentioned this pull request Jul 30, 2020
@prabirshrestha
Copy link
Contributor

prabirshrestha commented Jul 30, 2020

Latest tide contains the builder pattern so would be good to include for those too. I seem to be using builder pattern with render more. Here is the code feel free to copy paste.

https://github.com/prabirshrestha/rblog/blob/master/src/renderer.rs

examples/tide/src/main.rs Outdated Show resolved Hide resolved
examples/tide/src/main.rs Outdated Show resolved Hide resolved
@kaj
Copy link
Owner Author

kaj commented Jul 30, 2020

Thanks for the review, @prabirshrestha !

Latest tide contains the builder pattern so would be good to include for those too. I see to be using builder pattern with render more. Here is the code feel free to copy paste.

Ah, it's new? So that's why you didn't use it in the code you contributed, I was wondering. Then I'll change the code to use it.

@prabirshrestha
Copy link
Contributor

builder pattern was only introduced in 0.12. My PR was before that version hence didn't include the builder example.

Tide uses a diffrent flavor of Mime from the other frameworks
I have tried.  Add support for that.
@kaj
Copy link
Owner Author

kaj commented Jul 31, 2020

Since tide provides the http-types crate as tide::http, I'm considering a feature tide that exposes the same functionality as the http-types feature, but without the need for a direct dependency on http-types. Since tide is still in 0.x, it would probably be a tide012 feature, to make room for breaking changes.

And if I do that, it might make sense to move the traits and impls in ructe_tide.rs from the example to the feature.

kaj added 2 commits July 31, 2020 20:31
As a separate static file.  Includes some refactoring of the
static_file handler.
@kaj
Copy link
Owner Author

kaj commented Jul 31, 2020

I'm thinking of moving the contents of ructe_tide.rs into an actual tide feature of ructe, but before that: Two review questions for @prabirshrestha (and/or anyone else reading this) :

Is Render and RenderBuilder good names for the trait? Or would RucteResponse and RucteBuilder be better since it would be more clear what they would be about?

What should the Builder variant of the render methods return? For error handling, it might be better to return an std::io::Result<ResponseBuilder> and let the caller handle any errors? Or the render and render_html methods could finalize he builder, returning a tide::Response? That way, it could handle errors by returning a response containing the actual error rather than logging the error and returning a "generic" internal server error response.

@kaj
Copy link
Owner Author

kaj commented Jul 31, 2020

Another thing I have realised: the tide framework seems to make it possible to write the response body directly onto the response buffer (the same buffer as the response header is written to) rather than to a separately allocated buffer which is then copied onto the response buffer. I think that is worth some consideration.

Followup: Probably not worth doing. For one, that would hide the length of the body from the impelementation, forcing it to either use some extra buffer anyway or fall back to chunked encoding. Also, creating a Body from a &'static [u8] also copies the data to an owned vec in the Body.

So far, just the http-types feature through the tide reexport.
@kaj kaj changed the title Larger tide example. Tide feature and improved example Aug 2, 2020
@prabirshrestha
Copy link
Contributor

adding @yoshuawuyts for thoughts on this.

I do like Render and RenderBuilder as name.

+1 for streaming directly to response stream.

current PR is already is good state and given it is an example and no one will be taking a dependency on it I don't have strong opinions to make sure we get the apis right now.

kaj added 2 commits August 6, 2020 19:27
Since tide is still 0.x, it may be good to prepare for
breaking changes.
@kaj
Copy link
Owner Author

kaj commented Aug 7, 2020

Yes, you are right. Time to stop fiddling with details and merge this.

@kaj kaj merged commit 406e69b into master Aug 7, 2020
@kaj kaj deleted the more-tide branch August 7, 2020 22:32
@prabirshrestha
Copy link
Contributor

@kaj could you release a new version so I can take dependency on the new feature.

@kaj
Copy link
Owner Author

kaj commented Aug 14, 2020

@kaj could you release a new version so I can take dependency on the new feature.

Yes. I intended to get #88 (join_html and join_to_html) done before the release, but that was harder than I thought, so I'll make a relase (hopefully in a few hours) without it.

kaj added a commit that referenced this pull request Aug 14, 2020
* PR #80 and #94: Support Tide framework by a feature and an example.
* PR #91: Update basic examples to edition 2018.
* Issue #68, PR #90: Don't eat whitespace after a for loop.
* Issue #66, PR #89: Fix parse error for nested braces in expressions.
* PR #84: Use std::ascii::escape_default.
* PR #87: Provide ToHtml::to_buffer()
* Forbid unsafe and undocumented code.
* The build is on https://travis-ci.com/kaj/ructe now.
* Internal cleanup.

Thanks to @Aunmag and @prabirshrestha for reported issues and
contributed code.
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.

None yet

2 participants