Layout/ClassStructure and private constants

[marcandre]

I believe the following code should not raise an offense:

class Foo
  def foo
  end

  BAR = 42
  private_constant :BAR

  def bar
    # use BAR
  end
end

This is made worse by autocorrection that only moves the constant and not the private_constant just following it.

[nguyenquangminh0711]

@marcandre Note on this issue. Ruby also has public_constant. Those lines of codes are totally valid.

class Hello
  BAR = 10
  private_constant :BAR

  private

  FOO = 10
  public_constant :FOO
end

In this case, I think the behavior would be:

• Ignore constants which have private_constant calls somewhere.
• Add offeneses when meet normal constants or constants with public_constant not in the right place.

Similarly, methods have the same methods, including: private, public, private_class_method, public_class_method. However, I think that methods would be their own places, the same as normal private/public method. So, those cases would be caught.

class A
  class << self
    def a_public_class_method; end

    private

    def a_private_class_method; end
  end

  public_class_method :a_private_class_method
  private_class_method :a_public_class_method

  def a_public_method; end

  private :a_public_method

  def another_public_method; end

  private

  def a_private_method; end

  public :a_private_method
end

A.a_private_class_method
A.new.a_private_method

A.new.another_public_method

A.a_public_class_method # Raise exception
A.new.a_public_method # Raise exception

Should we tackle them too? I would love to work on this.

[marcandre]

Sorry I didn't reply earlier, @nguyenquangminh0711

1. I would not deal with methods and constants in the same cop.
2. private does not influence the visibility of constants. The only use of public_constant is to change the visibility of a constant that was made private with private_constant.
3. I don't know the multiple cops we have, there's probably one to add offenses for constants defined after a private that lack a private_constant, but I'm not sure; that could be useful.

If you want to work on this, I would recommend to first address this current issue, where Layout/ClassStructure should ignore constants that are made private with private_constant. Interested?

[thearjunmdas]

@marcandre - Is this still an issue? I tried to recreate this, but there were no offense generated for the stated code.

[marcandre]

Definitely an issue. I have a big PR that I need to complete, which addresses this: #9575

[thearjunmdas]

@marcandre Just for my clarity. So this is what i am doing and I am unable to recreate the issue :

1. Forked repo, build rubocop using gem build - master branch
2. Installed the rubocop i built and ran it for the following file:

class Foo
  def foo
  end

  BAR = 42
  private_constant :BAR

  def bar
    # use BAR
  end
end

Rubocop output:

layout_class_structure.rb:1:1: C: Style/Documentation: Missing top-level class documentation comment.
class Foo
^^^^^
layout_class_structure.rb:1:1: C: [Correctable] Style/FrozenStringLiteralComment: Missing frozen string literal comment.
class Foo
^
layout_class_structure.rb:2:3: C: [Correctable] Style/EmptyMethod: Put empty method definitions on a single line.
  def foo ...
  ^^^^^^^

System info:

❯ ruby -v
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-darwin19]                                                                                                          /0.0s
❯ rubocop -v
1.17.0  

[marcandre]

@thearjunmdas Thanks for checking. I'm not sure what the default is anymore (and too 😴 to check). Probably my example should be updated, but I still believe ClassStructure cop does not have any idea of what a private constant is. It's actually quite tricky to do. My PR handles it.

[thearjunmdas]

@marcandre - Thanks for the response. I guess your PR should do, I am new to the repo, so was wondering if i was going about it the right way. Thanks again :D Also i decided to give Recognize shareable_constant_value magic comment in Style/MutableConstant a shot as my first issue.