Reduce method visibility where possible

[fuadsaud]

Many methods that were marked as protected didn't actually need to be so.
This commit reduces the visibility level of these methods to private as
possible.

[coveralls]

Coverage increased (+0.0%) when pulling 343c3e6 on fuadsaud:protected-methods into 9e64c3f on lotus:master.

[coveralls]

Coverage increased (+0.0%) when pulling 343c3e6 on fuadsaud:protected-methods into 9e64c3f on lotus:master.

[coveralls]

Coverage increased (+0.0%) when pulling 65325d9 on fuadsaud:protected-methods into 9e64c3f on lotus:master.

[jodosha]

@fuadsaud thanks for this PR.

Before to proceed, we need to evaluate case by case the methods. For instance, by declaring response as private, we're no longer allowed to call invoke it as self.response, which may sound a little bit counter intuitive.

Also, the test suite doesn't exercise cases like these. We need, to ensure that the "natural" usage is preserved.

class VisibilityAction
  include Lotus::Action

  def call({})
    self.body = 'hi'
    self.body # void call to check that it doesn't raise an error
    body # same for this
  end
end

What do you think?

[fuadsaud]

Yes, indeed; let's discuss it on the diffs.

As a side note: this explicit receiver behaviour is a little weird since, there's an exception to it (explicit self on setter methods). I guess a more interesting approach would be to consider the explicit self receiver the exception to the private/protected method rule. But that's how it is ¯_(ツ)_/¯

[jodosha]

@fuadsaud the explicit receiver is necessary for another case: distinguish between a getter and a local variable.

class ExplicitReceiverAction
  include Lotus::Action

  def call(params)
    body = ComplexElaboration.run
    body # returns the result of ComplexElaboration
    self.body # returns the value of @body
  end
end

[jodosha]

@fuadsaud do you think this is still something that we want to work on? If yes, would you mind to write tests to ensure that explicit receiver would work as expected? I can understand your objection, but we are just two people and this framework will be used in the wild with unforeseen use cases. POLA FTW ;)

[fuadsaud]

Yes, I can, though not sure when I'll have time to do it (if anyone wants to step in and get it done, feel free). Also, do you mind pointing out which methods you think should be left as protected? Thanks.

[jodosha]

@fuadsaud sure, I'll apply the help-wanted label, then. Thanks!

[coveralls]

Coverage remained the same when pulling 743a149 on fuadsaud:protected-methods into cfe555e on lotus:master.

[jodosha]

@fuadsaud Looks good, but despite the fact that your implementation is formally correct, I'd amend with a few changes, in order to not surprise end users.

Look at the following code, where we need to use self for setting values like body, status and format, so people may be tempted to use self.headers (which is possible) and also self.cookies etc..

In other words, I'd not sacrifice correctness for usability here. Your PR is an improvement and rationalizes what should be private and protected, but we can't foresee how people will use those methods in their implementations.

require 'lotus/controller'
require 'lotus/action/cookies'
require 'lotus/action/session'

class MyAction
  include Lotus::Action
  include Lotus::Action::Cookies
  include Lotus::Action::Session

  self.configuration.handle_exceptions false

  def call(params)
    self.body   = 'x'
    self.status = 201
    self.format = :json

    self.headers.merge!('X-Custom' => 'OK')

    # OK to keep them private
    # self.configuration
    # self.finish

    # NOT OK (I'd keep them protected)
    # self.response
    # self.cookies
    # self.session
  end
end

action = MyAction.new
puts action.call({}).inspect