Skip to content
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

Layout/AccessModifierIndentation requires odd indentations #5956

Closed
jodosha opened this issue Jun 6, 2018 · 12 comments · Fixed by #6573
Closed

Layout/AccessModifierIndentation requires odd indentations #5956

jodosha opened this issue Jun 6, 2018 · 12 comments · Fixed by #6573

Comments

@jodosha
Copy link

jodosha commented Jun 6, 2018

To make Layout/AccessModifierIndentation to pass, I need to write code with odd indentation levels.


Expected behavior

I don't expect any violations for the snippets below.

Actual behavior

There are violations.

Steps to reproduce the problem

Example 1

Existing code

RSpec.describe Params do

  klass = Class.new(Params) do
    attr_reader :params

    private def export # this is where the violation happens
      super.select { |k, _| k.to_s.start_with?("f") }
    end
  end

  # ...
end

Code that satisfies Layout/AccessModifierIndentation

RSpec.describe Params do

  klass = Class.new(Params) do
    attr_reader :params

            private

    def export
      super.select { |k, _| k.to_s.start_with?("f") }
    end
  end

  # ...
end

Example 2

Actual code

RSpec.describe "..." do
  let(:action) do
    Class.new do
      include Hanami::Action

      prepend(Module.new {
        def _run_before_callbacks(params)
        end
        private :_run_before_callbacks
      })
    end.new
  end

# ...
end

Code that satisfies Layout/AccessModifierIndentation, BUT it leads to a violation of Layout/IndentationConsistency

RSpec.describe "..." do
  let(:action) do
    Class.new do
      include Hanami::Action

      prepend(Module.new {
        def _run_before_callbacks(params)
        end
                private :_run_before_callbacks
      })
    end.new
  end

# ...
end

RuboCop version

$ rubocop -V
0.57.0 (using Parser 2.5.1.0, running on ruby 2.4.3 x86_64-darwin16)
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 8, 2018

//cc @brandonweiss

@brandonweiss
Copy link
Contributor

@jodosha Hey! You’ve left out some useful information, like what your configuration is for the relevant cops (Layout/AccessModifierIndentation and Style/AccessModifierDeclarations)? And does this only occur when you’re meta-programming/dynamically creating classes/modules or does this happen when creating classes/modules with keywords (the “normal” way)?

Hmm. 🤔 I can imagine what might be going wrong here… Offhand, it seems like we probably want to alter Layout/AccessModifierIndentation to lint differently (or perhaps not lint at all?) if Style/AccessModifierDeclarations is configured with inline

@jodosha
Copy link
Author

jodosha commented Jun 8, 2018

@brandonweiss Thanks for getting back.


AFAIK these meta-programming examples are the only ones where I get the Layout/AccessModifierIndentation violations.

I don't have any configuration for Layout/AccessModifierIndentation and Style/AccessModifierDeclarations. For now they are disabled via configuration file because of these failures.


To use the EnforcedStyle: inline for Style/AccessModifierDeclarations isn't possible because the team style is to use private inline, but as a group modifier. The examples posted here are an exception.


Anyway, even enabling the inline style, I still get a Layout/AccessModifierIndentation violation for the cases posted here.

@brandonweiss
Copy link
Contributor

@jodosha I'm confused as to what you mean… when you say you “don’t have any configuration” for those cops, do you mean like, now you don't, because you disabled them? Or do you mean before you didn't have an explicit configuration set? Because if the cops are enabled then they are “configured” with the defaults. So you were using the defaults?

I’m not sure what “the team style is to use private inline, but as a group modifier” means? Can you explain and give an example?

@jodosha
Copy link
Author

jodosha commented Jun 8, 2018

@brandonweiss Let me explain: by upgrading to 0.57.0 there are "implicit" settings that the rubocop gem ships. So I tried with default settings, and got the failures described by this ticket.

Because we use Rubocop in our CI, in order to avoid build failures, I disabled these two cops in our local .rubocop.yml.


I’m not sure what “the team style is to use private inline, but as a group modifier” means? Can you explain and give an example?

The default team style is:

class Foo
  def call(input)
  end

  private

  def a_private_method
  end
end

I mentioned this, because it prevents us to use Style/AccessModifierDeclarations with inline.

@cupakromer
Copy link
Contributor

cupakromer commented Jun 14, 2018

I just ran into this as well. For me, this occurs with:

Layout/AccessModifierIndentation:
  EnforcedStyle: outdent

Style/AccessModifierDeclarations:
  Enabled: false

The code where issues occur is for meta programmed methods (changing a parent class / module method access or for methods defined via alias_method). The team uses group style normally, but for these single methods the modifier is indented to be inline with regular method code b/c it is not defining the start of a group:

module SomeModule
  include AnotherModule

  alias_method :another_name, :orig_name
  module_function :another_name # violation happens here

protected

  def any_protected_method
    # ...
  end
end

This behaved as expected before 0.57.0.

@pt-stripe
Copy link

I ran into this as well now with the inverse of @cupakromer :

Layout/AccessModifierIndentation:
  EnforcedStyle: indent

Style/AccessModifierDeclarations:
  EnforcedStyle: inline

This incorrectly outdents:

                    doc.to_xml
                  end

-                 private def root_xpath
-                   r = self.class.root_xpath
-                   raise "#{self.class}.root_xpath is not defined!" unless r
-                   r
-                 end
+    private def root_xpath
+      r = self.class.root_xpath
+      raise "#{self.class}.root_xpath is not defined!" unless r
+      r
+    end

And this one


 module MyModule
   class MyClass
-    private def prepare_transaction(tb, record)
-      tb.set_metadata(settlement: record.id)
+  private def prepare_transaction(tb, record)
+    tb.set_metadata(settlement: record.id)

@AlexWayfer
Copy link
Contributor

My case with RSpec:

stub_const(
  'Foo', Class.new do
    private

    def foo; end
  end
)

@deivid-rodriguez
Copy link
Contributor

I tried to fix this in #6573. Can you double check if that fixes the issue for you?

@AlexWayfer
Copy link
Contributor

Can you double check if that fixes the issue for you?

I have 3 disabling of this cop in Formalism and I have Unnecessary disabling for all of them with your version.

@deivid-rodriguez
Copy link
Contributor

That means it's working, right?

@AlexWayfer
Copy link
Contributor

That means it's working, right?

Yep! For my case. Thank you.

bbatsov pushed a commit that referenced this issue Dec 23, 2018
… modules (#6573)

* [Fix #5956] More robust AccessModifierIndentation

Use `end`'s indentation as the base indentation, which is more robust
since it also plays nice with dynamic module or class definitions.

* Move `effective_column` method to `RangeHelp`

It fits better in there an it also make it possible to reuse the method
in places where `ConfigurableEnforcedStyle` has been separatedly mixed
in.

* Extract `column_offset_between` method

And move `effective_colum` under the "private note".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants