Skip to content

Conversation

gammons
Copy link

@gammons gammons commented May 21, 2014

I found it slightly odd that I needed to always write a .call method in my controllers when that method was empty. So instead I decided to check for the existence of the super method in the parent class first.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 358710a on gammons:do_not_require_controller_call into 8d39557 on lotus:master.

@jodosha
Copy link
Member

jodosha commented May 21, 2014

@gammons Thanks for this PR. What's the use case? I mean, if you don't need to implement #call why do you need an action in the first place?

@jodosha
Copy link
Member

jodosha commented May 21, 2014

@gammons RE test:

class ActionWithoutCall
  include Lotus::Action
end

it "doesn't require #call implementation" do
  action     = ActionWithoutCall.new
  code, _, _ = action.call({})

  code.must_equal(200)
end

@sidonath
Copy link
Contributor

@jodosha FWIW I find this useful as well since sometimes all logic can happen in callbacks or there's no logic at all (e.g. rendering a dumb template)

@gammons
Copy link
Author

gammons commented May 21, 2014

@jodosha What @sidonath said ;) I have a landing page that had no need for any controller logic.

Also, thank you for that recommendation for a good test. The action was not raising an error, but rather it was throwing a 500. So I was able to validate my change with a spec, which I added above.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 787d77e on gammons:do_not_require_controller_call into 8d39557 on lotus:master.

@gammons gammons changed the title do note explicitly require Controller#call method Do not explicitly require Controller#call method May 24, 2014
@jodosha
Copy link
Member

jodosha commented Jun 26, 2014

@gammons @sidonath I've evaluated this issue, if we allow to omit it there are two downsides:

  1. Because #call is part of the public interface, future or third part modules (Lotus::Action::Something) should always check if the action respond to it. That potentially leads to several checks in the same request cycle.
  2. I registered a degradation of ~100 reqs/s
# Master

% wrk -t 2 http://localhost:9292/                
Running 10s test @ http://localhost:9292/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.87ms   12.05ms  51.85ms   92.54%
    Req/Sec     3.06k     1.02k    4.33k    85.34%
  57151 requests in 10.00s, 5.01MB read
Requests/sec:   5717.08
Transfer/sec:    513.68KB

# This branch

% wrk -t 2 http://localhost:9292/
Running 10s test @ http://localhost:9292/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     5.19ms   14.04ms  66.17ms   93.50%
    Req/Sec     3.01k     0.98k    4.33k    83.94%
  56055 requests in 10.00s, 4.92MB read
Requests/sec:   5605.63
Transfer/sec:    503.66KB

@jodosha jodosha closed this Jun 26, 2014
@fuadsaud
Copy link
Contributor

The empty method definition could be easily extracted out to a mixing in your application, right?

@jodosha
Copy link
Member

jodosha commented Jun 30, 2014

@fuadsaud yes, so if a developer adds that mixin, it would solves the problem that @gammons raised.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants