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

Match HTTP::Response params and return the output of the route #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

samsheff
Copy link

It seems the params of HTTP::Response#new have changed, and frank hasn't been updated. By removing the protocol header argument in the method call (which matches the HTTP lib), frank compiles and runs.

I also made a change that allows the body to return the output of the route, rather than a hardcoded "OK".

Edit:
The second commit fixes an error where there are two response bodies returned.

@samsheff
Copy link
Author

I also made a templating language similar to ERB that works with Frank, which is here: https://github.com/samsheff/Bunny

Let me know your thoughts on that as well! :)

@bcardiff
Copy link

Hi @samsheff , did you know there is support for ECR in crystal?

I think there are no samples in the repo other that in the specs, but you can see the Changelog 0.2.0 or search for ecr_file macro.

The ECR works by generating a class so we have compile time checks of the template :-)

ecr source code

@waj
Copy link
Contributor

waj commented Sep 23, 2014

Hey! your contribution came really fast after our chat in irc ;-)

It's true the HTTP::Response api changed but the change you did here is not correct. The "OK" wasn't the body but the response status message. Now the response initializer receives the status code (mandatory) then the body, headers, status message and http version (all optional hence usable with named arguments).
Also the HTTP::Response class now have some helper methods ok, not_found and error to build common server responses. Please take a look to this class: https://github.com/manastech/crystal/blob/master/src/net/http/common/response.cr

Regarding the templating language (and as @bcardiff mentions as I'm writting this response :-P) please take a look to the ECR templates built in the std library in Crystal. However your approach have the advantage of not having to recompile the application every time you make a change in the template but ECR would be tons of times faster and more desirable I think, in a production environment.

@samsheff
Copy link
Author

@waj @bcardiff Thanks for your feedback! In regards to ECR, I was looking for exactly that - But couldn't find it. I'll make corrections to the code I submitted to match @waj 's comments later tonight and you can let me know what you think. :)

@samsheff
Copy link
Author

@waj @bcardiff Can you take a look at the latest commit and let me know if this is ok? It uses the helper methods and supports 404's.
Also, I apologize if there needs to be additional work, I'm still learning my way around. :)

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.

3 participants