feat: add Layout/AccessModifierIndentation

[ferranpm]

What is the goal?

To standardise how private sections are indented in a class
While navigating Simba, I found a couple of classes with the private section indented (e.g. ServiceItems::ServiceItemStates::HandlerJob) and found that Rubocop did not complain.
We've usually used the outdented version

class Blop
  def initialize
  end

private

  def private_method
  end
end

This PR just adds the cop that looks for that outdentation.

Is this a restricting or expanding change?

RESTRICTING change

How is it tested?

Manual tests

[gorka-sq]

This is a great opportunity to test out the drafted flow, thanks! 🙌🏻

The proposal is to add Severity: info to restricting changes, allowing some time for teams to adopt new restricting agreements on our style. Some months later, we can remove the Severity: info thingy to make the change mandatory.

We need to settle things and communicate changes to the teams before moving forward with the PR, but it would be great to have your early feedback on the proposal ❤️

[ferranpm]

Thanks! Updated :)

[valdis]

i don't think that we need to make this severity: info, because rubocop checks are performed only on the changed files, so the impact is scoped to the code that you are workin on.
I think we all have allready agreed that we want uniform styling of the code everywhere, so adding yet another rule that slightly changes some of the code's formatting has minimal impact on our workflow.

[gorka-sq]

@valdis not all applications have rubocop configured to run on changed files only, though that is the case for most of them. We want to make sure these style changes are as non-disruptive as possible to everyone, and that's why we proposed to use severity: info for restricting changes, making them mandatory only after teams have had enough time to update.

Maybe we should consider using non-full scans in all our repositories, though.

[oinak]

I have a concern.

Right now the rule that wants private methods indented relative to private is not activating, but if you apply the fix for the new rule and indent private it will:

The changes will not be like

-private
+  private

but like

-private
+  private

-  def any_other_method
-    here
-  end
+    def any_other_method
+      here
+    end

-  def below
-    here
-  end
+    def below
+      here
+    end

[tomtomecek-es]

💯

[ferranpm]

I have a concern.

Right now the rule that wants private methods indented relative to private is not activating, but if you apply the fix for the new rule and indent private it will:

The changes will not be like

-private
+  private

but like

-private
+  private

-  def any_other_method
-    here
-  end
+    def any_other_method
+      here
+    end

-  def below
-    here
-  end
+    def below
+      here
+    end

I'm not sure I completely follow your scenario, so I've come up with the weirdest indentation and it seems to work for me:

fpelayo in ~/Workspace/sequra-style at M211 on access-modifier-indentation
> cat Bipbop.rb
class Bipbop
  def the_public_thing
    "This is public"
  end

    private

def the_private_thing
  "This is private"
end
end

fpelayo in ~/Workspace/sequra-style at M211 on access-modifier-indentation
> bundle exec rubocop -A Bipbop.rb 1>/dev/null 2>&1

fpelayo in ~/Workspace/sequra-style at M211 on access-modifier-indentation
> cat Bipbop.rb
class Bipbop
  def the_public_thing
    "This is public"
  end

private

  def the_private_thing
    "This is private"
  end
end

Is this not the case you're concerned about?

[valdis]

I have a concern.

Right now the rule that wants private methods indented relative to private is not activating, but if you apply the fix for the new rule and indent private it will:

The changes will not be like

-private
+  private

but like

-private
+  private

-  def any_other_method
-    here
-  end
+    def any_other_method
+      here
+    end

-  def below
-    here
-  end
+    def below
+      here
+    end

Rules that force code re-formating are anoying, but they annoy us only for a limited time. Thus i don't see this being a problem.
If we agree that this is the way how we want all our code formated, then let's set the rules and enforce them right away so each new PR we do would be forced to adapt this change.