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

Shorten inspect on ActionDispatch::Request #40173

Merged
merged 1 commit into from Sep 5, 2020
Merged

Shorten inspect on ActionDispatch::Request #40173

merged 1 commit into from Sep 5, 2020

Conversation

p8
Copy link
Member

@p8 p8 commented Sep 3, 2020

Summary

Calling request in an action of a controller generates an endless stream of
characters, including the Rack app and middlewares.
This can be frustrating when using a debugger in a controller and
accidentally calling request generates output for a couple of seconds.

Inspect on ActionDispatch::Request is shortened to relevant attributes:

"#<ActionDispatch::Request request_method=POST, original_url=https://glu.ttono.us/path/of/some/uri?mapped=1, remote_ip=1.2.3.4, media_type=application/x-www-form-urlencoded>"

Other Information

This pull request splits an older pull request into two separate pull requests.
#39439

Edit:
I've changed the output to use the same format as used for requests in the logs:

"#<ActionDispatch::Request POST "https://example.com/path/of/some/uri?q=1" for 1.2.3.4>"

@rails-bot rails-bot bot added the actionpack label Sep 3, 2020
Comment on lines 1305 to 1308
assert_match %r(#<ActionDispatch::Request method="POST"), request.inspect
assert_match %r(original_url="https://glu.ttono.us/path/of/some/uri\?mapped=1"), request.inspect
assert_match %r(remote_ip="1.2.3.4"), request.inspect
assert_match %r(media_type="application/x-www-form-urlencoded">), request.inspect
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert_match %r(#<ActionDispatch::Request method="POST"), request.inspect
assert_match %r(original_url="https://glu.ttono.us/path/of/some/uri\?mapped=1"), request.inspect
assert_match %r(remote_ip="1.2.3.4"), request.inspect
assert_match %r(media_type="application/x-www-form-urlencoded">), request.inspect
assert_equal '#<ActionDispatch::Request method="POST", original_url="https://glu.ttono.us/path/of/some/uri?mapped=1", remote_ip="1.2.3.4", media_type="application/x-www-form-urlencoded">', request.inspect

Small suggestion but I think we can just test the entire string without having to regexp match on parts of it.

Copy link
Member Author

@p8 p8 Sep 4, 2020

Choose a reason for hiding this comment

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

I think it's kind of long. Maybe the output of inspect can be shortened to just:
#<ActionDispatch::Request POST "https://glu.ttono.us/path/of/some/uri?mapped=1">

Copy link
Member Author

Choose a reason for hiding this comment

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

@tgxworld I've shortend the output to same format as used for requests in the logs. This also allows the assertion to use a single line without getting to long.

    "#<ActionDispatch::Request POST "https://example.com/path/of/some/uri?q=1" for 1.2.3.4>"

Calling request in an action of a controller generates an endless stream of
characters, including the Rack app and middlewares.
This can be frustrating when using a debugger in a controller and
accidentally calling `request` generates output for a couple of seconds.

Inspect on ActionDispatch::Request is shortened to the most relevant
attributes and uses the same format as used for request in the logs:

    "#<ActionDispatch::Request POST "https://example.com/path/of/some/uri?q=1" for 1.2.3.4>"
@kaspth kaspth merged commit 451c3c0 into rails:master Sep 5, 2020
@p8 p8 deleted the shorten-inspect-on-request branch September 8, 2020 09:02
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

4 participants