Action Order Cop

[mollerhoj]

Please excuse any obvious mistakes, this is my first cop.

Fix #540

[dvandersluis]

Thanks for the PR! This is missing a few required things, including tests, a changelog entry, adding a require to https://github.com/rubocop/rubocop-rails/blob/master/lib/rubocop/cop/rails_cops.rb, adding an entry for this cop to https://github.com/rubocop/rubocop-rails/blob/master/config/default.yml.

[dvandersluis]

Thanks for making the changes I requested. Looks good at this point, but still requires tests in order to be merged.

The missing elements I requested are there now.

[mollerhoj]

Thanks for all your help guys.

I implemented autocorrection, but I feel like I have to try it on our codebase before I feel I can trust it.

Also, I don't know how to mark it as a safe/unsafe correction (-a vs -A) or which it should be marked as.

[andyw8]

The only way I can think of that would be unsafe is if someone had accidentally defined the same method twice in a controller, so that only the later one was in use. Then re-ordering might change the behaviour.

[mollerhoj]

The only way I can think of that would be unsafe is if someone had accidentally defined the same method twice in a controller, so that only the later one was in use. Then re-ordering might change the behaviour.

Heh yeah, so it comes down to the significance of -A vs. -a I guess -a should never ever have any opportunity to go wrong, so we have to stick to -A because of this strange edgecase?

[andyw8]

Heh yeah, so it comes down to the significance of -A vs. -a I guess -a should never ever have any opportunity to go wrong, so we have to stick to -A because of this strange edgecase?

One approach that could make it safe would be to have the cop fail if multiple identical action names are detected.

[andyw8]

(normally Lint/DuplicateMethods would catch this, but that cop might be disabled).

[mollerhoj]

Yeah. I could complicate things by trying to check for duplicates here, and disable auto correction in that case? Though it seems quite pedantic, and could lead to less clear code.

[andyw8]

I'd suggest aiming for a first release without safe-correction, and then safe-correction can be addressed in a follow-up.

[koic]

aiming for a first release without safe-correction, and then safe-correction can be addressed in a follow-up.

Thank you for considering the corner case. However, whether it is an unsafe auto-correction is not determined by progress of implementation. This is determined by cop design.
https://docs.rubocop.org/rubocop/1.21/usage/auto_correct.html#safe-auto-correct

I think this cop can be safely designed due to changes to layout. So, if auto-correction is implemented, safe auto-correction (-a) should be the cop's implemented. If method names with the same name exist, the order must be guaranteed. e.g.

Before auto-correction

def edit
end

def index # first
end

def show
end

def index # second
end

After auto-correction

def index # first
end

def index # second
end

def show
end

def edit
end

Both should be remain for duplicate defined methods. So, I think it can make a design that compatible behavior. Can you add the above test case and for the implementation?

[mollerhoj]

Both should be remain for duplicate defined methods. So, I think it can make a design that compatible behavior. Can you add the above test case and for the implementation?

It's done! And thanks, we avoided an infinite loop by testing this corner case (> changed to >=).

[mollerhoj]

@pirj I feel like this one is good to go now?

[pirj]

Looks good overall 👍

I'd love to see a spec that shows how it behaves:

• when non-resourceful methods are present in the controller
• when private/protected methods are present

I personally don't mind turning this cop on by default (set it to pending now, and we'll flip it to enabled on a major release) - that's the reason the pending status was introduced, see https://docs.rubocop.org/rubocop/1.21/versioning.html#pending-cops
However, if you'd like to do so - please run the cop against real-world-rails to find out if there are any false positives or errors.
It's a good practice anyway to run newly introduced cops on a number of projects. We do this in rubocop-rspec at least.

[mollerhoj]

I'd love to see a spec that shows how it behaves:

• when non-resourceful methods are present in the controller

Do you mean spec/rubocop/cop/rails/action_order_spec.rb:70

• when private/protected methods are present

I just looked into it. I guess we'll have to only run this cop on public methods. Unfortunately, I see no simple way of detecting if a method is public or not with rubocop? The only way of implementing this I can think of is to manually read through the class, detecting any access_modifiers (private or protected) and turn the cop off unless the public access_modifier if detected? I fear this will increase complexity dramatically.

[mollerhoj]

I personally don't mind turning this cop on by default (set it to pending now, and we'll flip it to enabled on a major release) - that's the reason the pending status was introduced, see https://docs.rubocop.org/rubocop/1.21/versioning.html#pending-cops
However, if you'd like to do so - please run the cop against real-world-rails to find out if there are any false positives or errors.
It's a good practice anyway to run newly introduced cops on a number of projects. We do this in rubocop-rspec at least.

I'll look into it

[pirj]

we'll have to only run this cop on public methods

Makes sense.

no simple way of detecting if a method is public or not

There seems to be this and this, but frankly I never used any of them.

# 1.rb
class UserController < ApplicationController
  def commit; end
  def show; end
  private
  def index; end
end

# 2.rb
class UserController < ApplicationController
  def commit; end
  def show; end
  private def index; end
end

$ ruby-parse 1.rb
(class
  (const nil :UserController)
  (const nil :ApplicationController)
  (begin
    (def :commit
      (args) nil)
    (def :show
      (args) nil)
    (send nil :private)
    (def :index
      (args) nil)))
$ ruby-parse 2.rb
(class
  (const nil :UserController)
  (const nil :ApplicationController)
  (begin
    (def :commit
      (args) nil)
    (def :show
      (args) nil)
    (send nil :private
      (def :index
        (args) nil))))

For 2.rb it's possible to check if def isn't an argument to private, and for 1.rb skip everything below private (and protected). Hope those mixins would simplify the task.

[pirj]

when non-resourceful methods are present in the controller

Do you mean spec/rubocop/cop/rails/action_order_spec.rb:70

Please accept my apologies, missed that example completely 🙈

👍

[mollerhoj]

This PR for merge is ready AFAIK.

[koic]

This looks good to me. Can you use the new changelog entry format instead of CHANGELOG.md and squash your commits into one?
JFYI, the next release is scheduled for bug fix version 2.12.3. This PR is a new feature and will take some time to merge. Please wait for a while.

[koic]

@mollerhoj ping :-)

[pirj]

@mollerhoj ping

[mollerhoj]

@koic @pirj thanks for the pings. This should be good now?

[koic]

Thanks!