Skip to content

Conversation

lasseebert
Copy link
Contributor

Implements #66

  • Action::Request class that subclasses Rack::Request
  • Action#request method that returns a Action::Request instance.

I have written unit tests for the methods of Action::Request for which I think it makes sense to guarantee backwards compatibility.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 07c27c5 on lasseebert:request into * on lotus:master*.

@jodosha
Copy link
Member

jodosha commented Jan 2, 2015

@lasseebert This looks great! I'm going to merge it now, but love to hear your opinion about #accepted_encoding and #accepted_language.

Those two headers are weighted like Accept. For it, we have implemented a mechanism which evaluates those "q values" and returns an useful information. What if we move those two informations as private methods in Action::Rack and let those to provide a strict result?

Eg. for a request like this 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', #accepts returns "text/html".

What if we do the same here? 'HTTP_ACCEPT_LANGUAGE' => 'da, en;q=0.6', #language returns "da" and 'HTTP_ACCEPT_ENCODING' => 'gzip, deflate;q=0.6', #encoding returns "gzip".

@jodosha jodosha self-assigned this Jan 2, 2015
@jodosha jodosha added this to the v0.3.1 milestone Jan 2, 2015
@jodosha jodosha merged commit 07c27c5 into hanami:master Jan 2, 2015
jodosha added a commit that referenced this pull request Jan 2, 2015
@lasseebert
Copy link
Contributor Author

I think it's a great idea. I have never used anything but the first value in those headers. I can still imagine situations where it is useful to know the prioritized list of values (but without the q values):

  • language # => The top prioritzed language
  • languages # => Prioritized list of languages, e.g. ['da', 'en']
  • encoding # => The top prioritzed encoding
  • encodings # => Prioritized list of encodings, e.g. ['gzip', 'deflate']

@lasseebert lasseebert deleted the request branch January 2, 2015 12:12
@jodosha
Copy link
Member

jodosha commented Jan 2, 2015

@lasseebert I'd say to keep it consistent with MIME types logic: return only one value and expose a query method to understand if a value is accepted.

require 'lotus/controller'

class Show
  include Lotus::Action

  def call(params)
    # ...
    @_env['HTTP_ACCEPT'] # => "text/html,application/xhtml+xml,application/xml;q=0.9"

    accepts # => "text/html"

    accept?('text/html')        # => true
    accept?('application/xml')  # => true
    accept?('application/json') # => false
  end
end

So we could have #language, #accept_language? and #encoding, #accept_encoding? pairs.

@lasseebert
Copy link
Contributor Author

This makes sense if there is still access to the methods on Action::Request: #accept_language and #accept_encoding.

Otherwise we would hide the logic that parses the accept lists, and people who need it will probably create a Rack::Request, which was the point to avoid.

@jodosha jodosha mentioned this pull request Jan 6, 2015
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants