Methods to set content type and content #313

elorest · 2017-10-18T05:37:25Z

Description of the Change

Resolves discussion in this issue. #306

Adds respond_with block as a helper method to set content_types.

Returning values and render by itself still work unless a value has already been set via these methods.

Benefits

respond_with do
 html render("index.slang")
 json posts.to_json
 xml render("index.xml.slang")
 text "hello world"
end

Provides and easier and cleaner way to set content_types.

watzon · 2017-10-18T05:45:07Z

Very happy to see this being worked on so quickly

drujensen · 2017-10-18T13:30:19Z

src/amber/controller/render.cr

@@ -2,6 +2,24 @@ module Amber::Controller
  module Render
    LAYOUT = "application.slang"

+    protected def render_text(body, status_code = 200)


@elorest what do you think of dropping the render_ and just using text, json, html?

I think it would be better if their names are verbs, since verbs are more readable and nouns may collide with local variables.

yup, your right. It reads better.

I don't know how you all feel about it, but I like the idea of just using the normal render command with named params like Rails does.

def index render json: { key: "Value } end

IMHO render is already complicated enough. Also text and json aren't truly rendered, but rather just copied over into the response without changes. As these helper methods currently are they don't have to be macro's or complicate the already complex workings of the render macro.

I'm ok with that. @drujensen @eliasjpr @fridgerator WDYT?

@watzon that looks like a good solution.

Should it support the ability to detect the Accept header and allow you to define multiple response types?

respond_with( json: {"hello" => "world"}, text: "hello world", html: render("index.slang") )

I like the signature above is very intuitive. Crystal supports defining method with external names so the above can be written as

respond with: { "json" } def respond(with type) ... end

See External Names here https://crystal-lang.org/docs/syntax_and_semantics/default_values_named_arguments_splats_tuples_and_overloading.html

Yeah it's on my crystal lang coffee mug lol.

@eliasjpr In your code block above there's nothing that defines which type it's responding with though as the value of with is the json body.

I think that this makes more sense.

respond_with( json: {"hello" => "world"}, text: "hello world", html: render("index.slang") )

There's also the possibility of doing something like this

respond with: :json { json } # or respond with: :json do json end # or as above respond_with( json: {"hello" => "world"}, text: "hello world", html: render("index.slang") )

eliasjpr

LGTM

Additional questions?

eliasjpr · 2017-10-27T11:08:31Z

Was thinking about the new methods, they dont seem to be calling the render. What do you think of organizing them in such:

module Render
  module Helpers
    def render_text end;
    def render_html end;
    def render_json end;
  end 
end

And include them in the controllers...

…ber into is/render_content_types

elorest · 2017-10-29T07:27:44Z

Could you look over this and let me know if you see any issues? @drujensen @eliasjpr
I like that it's simple and easy to use, not sure if the accepts logic is missing anything though.

elorest · 2017-10-29T18:09:50Z

@eliasjpr There is already a module called render. I would just prefer to leave them in there so they're included with the render macros. We can talk about it though. I'm more concerned with the functionality right now.

eliasjpr

overall i like the implementation, i did left a comment after reading about default accept headers https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation/List_of_default_Accept_values

eliasjpr · 2017-10-29T19:13:29Z

src/amber/controller/render.cr

@@ -37,5 +37,43 @@ module Amber::Controller
        Kilt.render("#{{{path}}}/{{filename.id}}")
      {% end %}
    end
+
+    protected def respond_with(html : String? = nil, json : Hash | String? = nil, xml : String? = nil, text : String? = nil)
+      accepts = context.request.headers["Accept"].split(";").try(&.split(","))


There should probably be a guard check for when Accept is */*. also sometimes the accept header could be in this formattext/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Also we have some helper methods that you can make use of in this case, see https://github.com/amberframework/amber/blob/master/src/amber/support/mime_types.cr

@eliasjpr Thanks. I'm thinking that we should probably just default to html or text or something it doesn't match any of accept types. Seems like pretty much all browsers have / as the last set of accepts parameters.

How would you suggest the mime_type helpers could be used: It seems like Amber::Support::Mime.mime_type("json") is more complicated than "application/json". Since all of the ones we care about only point to one mime type I feel that it's probably bests just to hardcode the string. Thoughts?

Well, the Amber::Support::Mime has a Hash of mime-types you can probably write a nice matcher method in that module to match the one from the header, this will keep the code centralized in one place instead of hardcoding the string.

elorest · 2017-10-30T00:01:43Z

@drujensen @eliasjpr What would the expectation be when accepts and available response types don't match up? For instance it asks for text but the only thing available on the action is html

eliasjpr · 2017-10-30T00:47:26Z

src/amber/controller/render.cr

+    protected def respond_with(html : String? = nil, json : Hash | String? = nil, xml : String? = nil, text : String? = nil)
+      accepts = context.request.headers["Accept"].split(";").try(&.split(/,|,\s/))
+
+      if accepts.includes?("text/html") && html


Can this be a constant if you want to keep the string as such:

RESPOND = { html: "text/html", json: "application/json", text: "text/plain", xml: "application/xml" } accepts.includes? RESPOND[:html] accepts.includes? RESPOND[:json] ...

Yeah that looks good.

eliasjpr · 2017-11-03T18:06:28Z

src/amber/controller/helpers/render.cr

@@ -37,5 +37,30 @@ module Amber::Controller
        Kilt.render("#{{{path}}}/{{filename.id}}")
      {% end %}
    end
+
+    protected def respond_with(html : String? = nil, json : Hash | String? = nil, xml : String? = nil, text : String? = nil)


Can we remove this method?responders.cr does the format very nicely.

Yeah. Weird that shouldn't be there.

drujensen · 2017-11-03T23:33:47Z

src/amber/controller/helpers/render.cr

+      elsif xml
+        respond_with_xml(xml)
+      else
+        respond_with_text("Response not acceptable", 406)


I tried curl -H "Accept: application/pdf" http://localhost:3000 against this branch but for some reason the html was still returned instead of an error 406.

We decided that since all browsers include */* as a final parameter that if nothing matched up just to return the first defined content type.

drujensen

This is excellent work. I think we can make a couple improvements by allowing arrays and hashes to be passed into json but that can wait until its requested.

elorest · 2017-11-03T23:58:21Z

@drujensen It does allow for hashes and namedtuples right now. Just not nested.

…ber into is/render_content_types

watzon · 2017-11-04T06:44:33Z

Yay! Great job guys!

* methods to set content type and content * respond_with * respond with works * organized methods better * allow hash or json for json response * formatting * small changes * moved content_types out to constant * tested IRL * refactored file structure * works after many hours * good for now, got it working as good as it used to. * added tests * tests work and fixes empty headers' * removed vestigul code

* methods to set content type and content * respond_with * respond with works * organized methods better * allow hash or json for json response * formatting * small changes * moved content_types out to constant * tested IRL * refactored file structure * works after many hours * good for now, got it working as good as it used to. * added tests * tests work and fixes empty headers' * removed vestigul code Former-commit-id: 6d2d14d

methods to set content type and content

1e8a04a

elorest requested review from eliasjpr, drujensen, fridgerator, veelenga and a team October 18, 2017 05:37

elorest mentioned this pull request Oct 18, 2017

render text / plain text in controller response? #306

Closed

veelenga approved these changes Oct 18, 2017

View reviewed changes

drujensen approved these changes Oct 18, 2017

View reviewed changes

eliasjpr added the WIP label Oct 21, 2017

eliasjpr previously approved these changes Oct 25, 2017

View reviewed changes

eliasjpr added 2 commits October 24, 2017 21:23

Merge branch 'master' into is/render_content_types

fd9a719

Merge branch 'master' into is/render_content_types

e901a43

eliasjpr and others added 4 commits October 28, 2017 08:24

Merge branch 'master' into is/render_content_types

e4b48bb

respond_with

94a7c34

Merge branch 'is/render_content_types' of github.com:Amber-Crystal/am…

006fdbe

…ber into is/render_content_types

respond with works

7be1a54

elorest removed the WIP label Oct 29, 2017

elorest changed the title ~~Methods to set content type and content [WIP]~~ Methods to set content type and content Oct 29, 2017

elorest added 3 commits October 29, 2017 12:03

organized methods better

796f84a

allow hash or json for json response

f5698bf

formatting

5e540c8

eliasjpr reviewed Oct 29, 2017

View reviewed changes

small changes

a068f4a

eliasjpr reviewed Oct 30, 2017

View reviewed changes

eliasjpr added 2 commits October 31, 2017 11:03

Merge branch 'master' into is/render_content_types

5dd0796

Merge branch 'master' into is/render_content_types

77e1307

elorest mentioned this pull request Nov 2, 2017

Controller Helpers amberframework/jasper-helpers#8

Open

5 tasks

elorest added 6 commits November 2, 2017 00:03

Merge branch 'master' into is/render_content_types

555f2ee

moved content_types out to constant

60bce41

tested IRL

70b4813

refactored file structure

a6cbee5

works after many hours

73b65b6

good for now, got it working as good as it used to.

2493d73

eliasjpr reviewed Nov 3, 2017

View reviewed changes

elorest added 3 commits November 3, 2017 16:58

added tests

1478f5e

tests work and fixes empty headers'

560de4f

Merge branch 'master' into is/render_content_types

5efb25f

drujensen reviewed Nov 3, 2017

View reviewed changes

drujensen approved these changes Nov 3, 2017

View reviewed changes

removed vestigul code

1d60857

Merge branch 'is/render_content_types' of github.com:Amber-Crystal/am…

58791b1

…ber into is/render_content_types

eliasjpr approved these changes Nov 4, 2017

View reviewed changes

elorest merged commit 19574c1 into master Nov 4, 2017

eliasjpr deleted the is/render_content_types branch November 5, 2017 16:49

This was referenced Nov 12, 2017

Document merged PRs from the last month. #384

Closed

Pull Requests which need to be documented before release. #385

Closed

faustinoaq added this to Done in Framework 2018 May 5, 2018

faustinoaq removed this from Done in Framework 2018 Jun 12, 2018

faustinoaq added this to Done in Amber Framework 2017 Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Methods to set content type and content #313

Methods to set content type and content #313

elorest commented Oct 18, 2017 •

edited

watzon commented Oct 18, 2017

drujensen Oct 18, 2017

c910335 Oct 18, 2017

drujensen Oct 18, 2017

watzon Oct 18, 2017

elorest Oct 19, 2017

elorest Oct 20, 2017

drujensen Oct 21, 2017

eliasjpr Oct 21, 2017 •

edited

elorest Oct 23, 2017 •

edited

watzon Oct 25, 2017

eliasjpr left a comment

eliasjpr commented Oct 27, 2017

elorest commented Oct 29, 2017

elorest commented Oct 29, 2017

eliasjpr left a comment

eliasjpr Oct 29, 2017

elorest Oct 29, 2017

eliasjpr Oct 30, 2017

elorest commented Oct 30, 2017

eliasjpr Oct 30, 2017 •

edited

elorest Oct 30, 2017

eliasjpr Nov 3, 2017

elorest Nov 3, 2017

elorest Nov 3, 2017

drujensen Nov 3, 2017

elorest Nov 3, 2017 •

edited

drujensen left a comment

elorest commented Nov 3, 2017

watzon commented Nov 4, 2017

Methods to set content type and content #313

Methods to set content type and content #313

Conversation

elorest commented Oct 18, 2017 • edited

Description of the Change

Benefits

watzon commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasjpr Oct 21, 2017 • edited

Choose a reason for hiding this comment

elorest Oct 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasjpr left a comment

Choose a reason for hiding this comment

eliasjpr commented Oct 27, 2017

elorest commented Oct 29, 2017

elorest commented Oct 29, 2017

eliasjpr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorest commented Oct 30, 2017

eliasjpr Oct 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorest Nov 3, 2017 • edited

Choose a reason for hiding this comment

drujensen left a comment

Choose a reason for hiding this comment

elorest commented Nov 3, 2017

watzon commented Nov 4, 2017

elorest commented Oct 18, 2017 •

edited

eliasjpr Oct 21, 2017 •

edited

elorest Oct 23, 2017 •

edited

eliasjpr Oct 30, 2017 •

edited

elorest Nov 3, 2017 •

edited