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

make `as` option work with get parameters #25435

Merged
merged 1 commit into from Jun 25, 2016

Conversation

Projects
None yet
5 participants
@y-yagi
Member

y-yagi commented Jun 19, 2016

Currently, if path is a relative path, add format without the discrimination of the query.
Therefore, if there is a query, format at end of the query would been added,
format was not be specified correctly.

This fix add format to end of path rather than query.

@rails-bot

This comment has been minimized.

rails-bot commented Jun 19, 2016

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@y-yagi y-yagi force-pushed the y-yagi:make_as_option_work_with_get_parameters branch Jun 19, 2016

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 21, 2016

It seems URI.parse is not the fastest method call:

require 'benchmark/ips'
require 'uri'

uri = 'http://example.com/foos_json?foo=heyo'

Benchmark.ips do |x|
  x.report('URI.parse') { URI.parse(uri) }
end

From which I get:

Warming up --------------------------------------
           URI.parse     7.811k i/100ms
Calculating -------------------------------------
           URI.parse     89.649k (± 3.8%) i/s -    453.038k in   5.060840s

I think we'd need to check what impact parsing the URI has on the overall integration test performance. You can use the benchmarks from here to verify that: https://github.com/eileencodes/integration_performance_test.

@kaspth kaspth assigned kaspth and unassigned rafaelfranca Jun 21, 2016

@y-yagi

This comment has been minimized.

Member

y-yagi commented Jun 23, 2016

Thank you for pointing that out. I tried to check performance. Used script is here.

The results are as follows:

master

Warming up --------------------------------------
INDEX: Integration Test
                        76.000  i/100ms
Calculating -------------------------------------
INDEX: Integration Test
                        799.936  (± 8.6%) i/s -      4.028k in   5.074007s
Warming up --------------------------------------
CREATE: Integration Test
                        31.000  i/100ms
Calculating -------------------------------------
CREATE: Integration Test
                        322.260  (± 6.2%) i/s -      1.612k in   5.022279

this branch

Warming up --------------------------------------
INDEX: Integration Test
                        74.000  i/100ms
Calculating -------------------------------------
INDEX: Integration Test
                        783.904  (± 7.8%) i/s -      3.922k in   5.034598s
Warming up --------------------------------------
CREATE: Integration Test
                        31.000  i/100ms
Calculating -------------------------------------
CREATE: Integration Test
                        312.267  (± 7.7%) i/s -      1.581k in   5.095996s

Hmm. This has turned into a little performance degradation...

make `as` option work with get parameters
Currently, if path is a relative path, add format without the discrimination of the query.
Therefore, if there is a query, format at end of the query would been added,
format was not be specified correctly.

This fix add format to end of path rather than query.

@y-yagi y-yagi force-pushed the y-yagi:make_as_option_work_with_get_parameters branch to e130ce4 Jun 25, 2016

@y-yagi

This comment has been minimized.

Member

y-yagi commented Jun 25, 2016

I have modified to use URI.parse only if using as option. I think that it is this that there is no effect on existing tests. How right?

@kaspth kaspth merged commit 17c1c9f into rails:master Jun 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Member

kaspth commented Jun 25, 2016

Nice thanks! ❤️

kaspth added a commit that referenced this pull request Jun 25, 2016

Merge pull request #25435 from y-yagi/make_as_option_work_with_get_pa…
…rameters

make `as` option work with get parameters
@kaspth

This comment has been minimized.

Member

kaspth commented Jun 25, 2016

Backported to 5-0-stable: eb6efd1.

@y-yagi y-yagi deleted the y-yagi:make_as_option_work_with_get_parameters branch Jun 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment