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

Fix PMD violation CommentDefaultAccessModifier #5665

Closed
pbludov opened this issue Mar 30, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@pbludov
Copy link
Collaborator

commented Mar 30, 2018

PMD rule CommentDefaultAccessModifier

// suspicious class
public class Foo {
    final String stringValue = "some string";
    String getString() {
       return stringValue;
    }

    class NestedFoo {
    }
}

// should be
public class Foo {
    /* default */ final String stringValue = "some string";
    /* default */ String getString() {
       return stringValue;
    }

    /* default */ class NestedFoo {
    }
}

// or may be
public class Foo {
    private final String stringValue = "some string";
    private String getString() {
       return stringValue;
    }

    public class NestedFoo {
    }
}

The access modifier should be explicitly set to private, protected, public or /*default*/.

@romani

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

from: #5743 (comment)

A brief conclusion:
The most problematic part is the construcor in a private inner class:

public class Outer {
    private static class Inner {

        /* ??? */Inner() {
        }
}

Options:

  • private violates (PMD rule AccessorClassGeneration)[https://pmd.github.io/pmd-6.2.0/pmd_rules_java_bestpractices.html#accessorclassgeneration];
  • public violates Idea inspection 'public' constructor in non-public class;
  • protected violates nothing, but looks unobvious;
  • /* default */ just a waste of space.

================

from me:
usage of CommentDefaultAccessModifier is more preferable, as it worse engineers to write weird comments that will be easy to catch on code-review.

conflict is with
https://pmd.github.io/pmd-6.2.0/pmd_rules_java_bestpractices.html#accessorclassgeneration
is ok, I would rather deactivate accessorclassgeneration and enable CommentDefaultAccessModifier.

We need to forbid usage of package private, all should be private or public (one day we will refactor all "protected" too).
AccessorClassGeneration is similar to AccessorMethodGeneration, we are not at that level of optimization of our code to bother about such generated wrappers. We should more care about code that we see in file.

    <!-- We do not care about this minor overhead, we are not Android application and we do not
         want to change visibility of methods. Package-private visibility should be avoid almost
         always. -->
<exclude name="AccessorMethodGeneration"/>

@pbludov , please send us PR with privates and disabled AccessorClassGeneration.

@pbludov

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 24, 2018

Well, back to this issue after the next release of JaCoCo.
The JaCoCo issue jacoco/jacoco#660 is the stopper for this issue. This has already been fixed, we need to wait a little until the next release.

@romani

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

@pbludov , in this issue we speak about visibility modifiers, in jacoco issue discussion about enum/switch.
Do I miss smth ?

@pbludov

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 26, 2018

Do I miss smth ?

Yes, the JaCoCo dev turned this issue to a more general case:
jacoco/jacoco#660 (comment)

@romani

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@pbludov , thanks a lot, while we are waiting, can you send us PR with adding "public" to top level classes and "disabled AccessorClassGeneration". To allow us after release of jacoco, just to use "private" in all cases. Release of jacoco could take a while, it is better to minimize context of fix on our side, while we remember all details.

@pbludov

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2018

Since PMD 6.4.0 it is possible to use /* package */ pseudo-modifier. See pmd/pmd#1149

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

@pbludov Is it possible to come back to this and resolve it?

pbludov added a commit to pbludov/checkstyle that referenced this issue Mar 11, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Mar 12, 2019

romani added a commit that referenced this issue Mar 14, 2019

@romani romani added this to the 8.19 milestone Mar 14, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Fix is merged

@romani romani closed this Mar 14, 2019

Vantuz added a commit to Vantuz/checkstyle that referenced this issue Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.