Rob, let me know if this isn't a clean commit with just the changes to the Response.Out. I'm still trying to work out my git flow for handling a fork correctly. Even though, I closed the old request it looks like github remembers it and brings in comments from previously closed requests.

For github, a pull request is basically "merge the head of branch A into this other branch". It is not related to the particular commits you have. If you push more commits to a branch that has an open pull request, they will automatically be added to the pull request.

The typical flow is:

• Open a pull request
• Respond to comments by pushing another commit
• Repeat
• Merge

(as opposed to opening a new pull request for every round of review)

I hadn't realized about the name collision before, thanks for bringing that up.

HRM, it seems like a very confusing API to have Response implement methods WriteHeader and WriteStatusHeader. I think there must be a better way to achieve the default status/content type by Result type.

Play! 2 makes it part of the return value, e.g.

return c.Ok(c.Render(...)) 

but that seems sort of heavy handed.

Rails seems to have it as a parameter to its render method, which is difficult to pull off here since we don't accept a random map of stuff.

Maybe if the Results embedded a BaseResult with those two properties:

type BaseResult struct {
  Status int
  ContentType string
}

type RenderTemplateResult struct {
  BaseResult
  ..
}

func (c Controller) RenderTemplate(..) Result {
  ...
  return RenderTemplateResult{{200, "text/html"}, ...}
}

That may make a bit more sense than the current design of having every Result hardcode its default status code. For example, a RenderTemplateResult could be used to show error pages, and it's really at the construction site that you know what the status code should be.

Also, do you think there is any value in allowing callers to override the content type for a built-in Result? I'm having trouble recalling why I thought that was necessary..

Any thoughts?

I had some very similar thoughts this past weekend but got completely distracted with battling git. Besides rails, I've used play 2.0 with scala and was at first really put off by the whole Ok(...) thing but we do have some cases where we don't return an Ok and I found that going back months later it made the code very readable (which I cannot say for scala in general).

It's very much the rails way to set headers for you based on response types. The problem we get into though, suppose you have a route that includes a :wants .pdf, rails is going to automatically set the content-type to 'application/pdf' and write it out but what happens if the file isn't there and you need to return an html error message instead (clients don't always want to see a 404 and their may be other ways to handle it). In rails, you end up writing a controller method that duplicates much of the built-in functionality and is rather long and nasty. This is a bit of a convoluted case though, I don't see someone using angularjs and wanting to send back a non-json response.

I'll put some more thought into it over the next day or two and see if I have any further ideas.

In Play 2.x your BaseResult is named SimpleResult

case class SimpleResult(header: ResponseHeader, body: Enumerator[Array[Byte]],
    streamingStrategy: StreamingStrategy = StreamingStrategy.Buffer())

case class ResponseHeader(status: Int, headers: Map[String, String] = Map.empty)

Also in play (in scala, things are a bit different in java) you can do something like

Ok(<h1>Hello World!</h1>).as(HTML).withHeaders(CACHE_CONTROL -> "max-age=3600", ETAG -> "xx")

Both Play and Rails have the concept of a result and a response. In rails headers are part of the response object and the documentation even says it's unusual for the code to do anything with them. Play wraps the header access into the results (because it's really unusual to go down and mess with the netty response itself).

I had not noticed that in both play and revel there's an encapsulation concept where Controller extends Results which extends Response. Instead of starting at the bottom of the chain, I probably should have looked from the top down.

Not really advocating anything here, just really trying to put some ideas into the pipeline.

That's good information. I haven't used Play 2.0 actually - do all the result types extend SimpleResult? I suppose Revel could support exactly that API. It seems to be basically "builder" pattern vs simply setting properties. The builder seems a little bit nice, but it really expands the API surface a lot. I'm not sure if it's worth it. e.g.

c.Response.Status = http.StatusMovedPermanently
c.Response.ContentType = "text/plain"
return c.Render()

vs

return c.Render().
  Status(http.StatusMovedPermanently).
  ContentType("text/plain")

or

return c.MovedPermanently(c.Render()).
  ContentType("text/plain")

It shouldn't be super common for developers to override the default status code or content type, so I'm not sure I see the necessity of specifying the status code 100% of the time. (And if they did frequently want another status code, they could create a new factory method to construct it).

Sadly, I guess Play! doesn't provide any advice on how to deal with the default status code situation. It sounds like Rails has the same issue where the results have a default status code / content type, but Rails doesn't allow the developer to override them without duplicating? That seems poor.

The comments in the code for play 2.1 have quite a few deprecations and indications that SimpleResult will be the only ResultType in play 2.3.

For that single use case rails completely got in the way. In fact, it was one of two pieces that I split off into a play 2.0 based service late last year. The other piece was very performance bound and moving it off ruby/rails to play/scala gave it a huge boost. Those were also the first two things that I moved to go.

My original thought here was that I didn't like the way response.Out.Headers() read and wanted the syntax to be response.Headers() (which is very rails like). I've been wondering if that's really the place to put header access, should it be in the result instead? The way things are working is that the controller has a result and a response, with the response being the low-level way that result gets written to the client. Should header manipulation happen at the result instead so that there's little to no reason to do anything with the response object itself?

Also one of the things I noticed with play is that without a content type set it does content determination like http.ResponseWriter does. For both of them they look at the first 500ish bytes to determine what kind of content you have. I'm not sure if that's hugely useful and by just setting the content type you can avoid that code from running.

Good insight. I agree that in the normal case, the developer shouldn't need to touch the Response, which speaks to getting the headers into the Result.

That suggests that there should be no "Response" object, just the connections "ResponseWriter" and the action-returned "Result". And then the question turns to the right way to construct Results -- I'm not actually sure the builder pattern on an embedded BaseResult works as sketched earlier -- the return value would be the embedded BaseResult, not the overall Result. I wonder if this is how Play2 ended up with the wrapping approach :)

I guess the 3rd code snippet I gave above could work. However this turns out, I would like the wrapping to be optional since it is really not that common that you need a custom status code. (Perhaps depending on the application, but for simple form-driven web applications it's never needed)

Rob, I'm a bit over loaded this week, I'll do my best to revisit this later in the week.

@robfig I prefer your 2nd example because it flows a bit better when reading it. The 3rd example seems a bit disjointed.

@plamb What's the deal with this PR? Seems your code was well intentioned and the conversation here goes into great detail discussing the options.

@revel/core I feel this is a valuable discussion that needs a little life put into it. Thoughts?

This is somewhat of a significant decision as it will affect how content will be delivered in Revel.

I vote for a builder pattern that uses a default content-type in order to avoid dynamic content type detection. I think the builder pattern is a clean interface to control a results parameters.

That said, code readability is a good goal for Revel's API.

Now, I may be too tired to think, but what about not calling c.Render() at all by default? Couldn't we set the content of c.Result automatically in most cases with sane defaults and convention over configuration?

Closing this request as server engine will obsolete it