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

Add documentation to ActionDispatch::Request #9698

Merged
merged 1 commit into from
Mar 13, 2013

Conversation

garethrees
Copy link
Contributor

I've added some documentation to the following:

  • ActionDispatch::Request#fullpath
  • ActionDispatch::Request#original_url
  • ActionDispatch::Request#media_type

Could someone check that I've explained them correctly?

I also have a question about Request#media_type referring to it's documentation.

I created a simple blog and used respond_with so that the articles index responds to XML and JSON.

Here's what I get in the console:

app.get '/articles.xml'
app.request.media_type
 => "application/x-www-form-urlencoded" 

app.get '/articles.json'
app.request.media_type
 => "application/x-www-form-urlencoded"

Should I be seeing application/xml and application/json respectively?

steveklabnik added a commit that referenced this pull request Mar 13, 2013
Add documentation to ActionDispatch::Request
@steveklabnik steveklabnik merged commit 02caa02 into rails:master Mar 13, 2013
@steveklabnik
Copy link
Member

The docs look good, thank you.

@steveklabnik
Copy link
Member

Should I be seeing application/xml and application/json respectively?

Hmm, I am not sure, but yes, that looks funny to me.

# app.response.fullpath # => "/articles"
#
# app.get "/articles?page=2"
# app.response.fullpath # => "/articles?page=2"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.response? Shouldn't this be .request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I think so, yes. Brains are a funny thing. I'll fix that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh! Sorry! Obviously hadn't had enough coffee this morning. Just noticed another one…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5bf6d8e

@carlosantoniodasilva
Copy link
Member

I'm afraid people will look at the docs and ask themselves: "wtf is app?". I don't think the app should be mentioned in the docs.

# The +String+ MIME type of the request
#
# app.get "/articles"
# # => "application/x-www-form-urlencoded"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be:

 #    app.get "/articles"
 #    app.request.media_type
 #    # => "application/x-www-form-urlencoded"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3c8e4a

@steveklabnik
Copy link
Member

@carlosantoniodasilva What instead? Rails.application? I've seen app elsewhere, so it made sense to me, though I am admittedly outside of the norm. ;)

@carlosantoniodasilva
Copy link
Member

@steveklabnik I believe @garethrees has tested this in the console, where app is an instance of Rails' integration test class. I think it's not related with the request docs, so we should probably not mention it.

How about:

#    # assume a GET to "/articles"
#    request.fullpath # => "/articles"

Or just removing it seems ok:

#    get "/articles"
#    request.fullpath # => "/articles"

Wdyt?

@steveklabnik
Copy link
Member

I'm happy if you're happy. You can go ahead and make that change, I'm done screwing up this simple commit. :)

@garethrees
Copy link
Contributor Author

@garethrees has tested this in the console

Yup. Thought that was a common trick, but I agree it makes less sense when accessing request from a controller.

The main reason I was documenting it is because I kept having to check which would give me base_url / params etc.

While we're at it, what does original_fullpath do? Could also be worth clarifying original_url?

@carlosantoniodasilva
Copy link
Member

@steveklabnik hehe alright :)

@garethrees I think it is a common trick, it's just that might not be worth mentioning it in the docs regarding the request methods.

original_fullpath was added by @drogus to fix an engine issue iirc, but I don't remember the details, maybe @drogus does. This is the commit that added it: 482ec2a.

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.

None yet

3 participants