ParameterName: deprecate 'scope' and 'excludeScope' properties, introduce new property 'accessModifiers' #3675

Closed
romani opened this Issue Dec 24, 2016 · 14 comments

Comments

Projects
None yet
3 participants
@romani
Member

romani commented Dec 24, 2016

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
                        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="charset" value="UTF-8"/>
  <property name="severity" value="warning"/>
  <module name="TreeWalker">
    <module name="ParameterName">
      <property name="severity" value="warning"/>
      <property name="format" value="^([a-z][a-z][a-zA-Z])$"/>
      <property name="ignoreOverridden" value="false"/>
      <property name="scope" value="private"/>
    </module>
  </module>
</module>

$ cat MyClass.java 
public final class MyClass {
    /**
     * This is a method.
     * @author Name with dot.
     */
    public void foo(String publicPar){}

    /**
     * This is a method.
     * @author Name wihtout dot
     */
    private void foo2(String privatePar){}    
}

$ java -jar checkstyle-7.3-all.jar -c config.xml MyClass.java 
Starting audit...
[WARN] /var/tmp/MyClass.java:6:28: Name 'publicPar' must match pattern '^([a-z][a-z][a-zA-Z])$'. [ParameterName]
[WARN] /var/tmp/MyClass.java:12:30: Name 'privatePar' must match pattern '^([a-z][a-z][a-zA-Z])$'. [ParameterName]
Audit done.

scope is private, so expected violations are only on private methods.
the same for excludeScope.

Side note: when config is changed to work only with "public" - it works fine. Looks like scope is counted by more visible and less visible. But should be simply by matching.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 27, 2016

Contributor

@romani
Method matchScope uses isIn method which assumes that PUBLIC is a subscope of PRIVATE. That means that all methods with private and higher scopes will be checked. For now 'scope' parameter of the check sets the lower boundary of the scope to check.

But should be simply by matching.

Got the idea, however it will completely change check's behavior for scopes.

Contributor

MEZk commented Dec 27, 2016

@romani
Method matchScope uses isIn method which assumes that PUBLIC is a subscope of PRIVATE. That means that all methods with private and higher scopes will be checked. For now 'scope' parameter of the check sets the lower boundary of the scope to check.

But should be simply by matching.

Got the idea, however it will completely change check's behavior for scopes.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 27, 2016

Member

Yes, we have to change behavior completely to be clear on how it works.
That was recently requested to cover google rule, so we just need satisfy google rule

Member

romani commented Dec 27, 2016

Yes, we have to change behavior completely to be clear on how it works.
That was recently requested to cover google rule, so we just need satisfy google rule

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 27, 2016

Contributor

@romani
Just a few examples of how it will work:

  1. scope = public, format = ^h$
public void v1(int haaa) {} // violation
protected void v4(int haaa) {} // OK
void v2(int haaa) {} // OK
private void v3(int abc) {}  // OK
  1. scope = private, format = ^h$
public void v1(int haaa) {} // OK
protected void v4(int haaa) {} // OK
void v2(int haaa) {} // OK
private void v3(int abc) {} // violation
public void l() {
        FuncIfc l1 = (int lexp)->{}; // violation (scope is private)
        FuncIfc l2 = (limp)->{};  // violation (scope is private)
    }
  1. scope = anoninner, format = ^h$
public void v1(int haaa) {} // OK
protected void v4(int haaa) {} // OK
void v2(int haaa) {} // OK
private void v3(int abc) {} // OK
public void v1(int h) {
        new Object () {
            public void i(int inner) {} // violation
        };
}

Are they correct?

Contributor

MEZk commented Dec 27, 2016

@romani
Just a few examples of how it will work:

  1. scope = public, format = ^h$
public void v1(int haaa) {} // violation
protected void v4(int haaa) {} // OK
void v2(int haaa) {} // OK
private void v3(int abc) {}  // OK
  1. scope = private, format = ^h$
public void v1(int haaa) {} // OK
protected void v4(int haaa) {} // OK
void v2(int haaa) {} // OK
private void v3(int abc) {} // violation
public void l() {
        FuncIfc l1 = (int lexp)->{}; // violation (scope is private)
        FuncIfc l2 = (limp)->{};  // violation (scope is private)
    }
  1. scope = anoninner, format = ^h$
public void v1(int haaa) {} // OK
protected void v4(int haaa) {} // OK
void v2(int haaa) {} // OK
private void v3(int abc) {} // OK
public void v1(int h) {
        new Object () {
            public void i(int inner) {} // violation
        };
}

Are they correct?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 27, 2016

Contributor

@romani
And also take a look at our description for 'scope' parameter type
http://checkstyle.sourceforge.net/property_types.html#scope

The scope is treated inclusively (as javadoc does): 'package' means all 'package', 'protected' and 'public' methods/fields/classes.

For example, private means all private, package, protected, public. But for this check it will become incorrect description as private will mean only private scope. Maybe scope and exludeScope should become String parameters and declare the name of the scope. Not sure whether we can use 'scope' as a parameter type for this check.

Contributor

MEZk commented Dec 27, 2016

@romani
And also take a look at our description for 'scope' parameter type
http://checkstyle.sourceforge.net/property_types.html#scope

The scope is treated inclusively (as javadoc does): 'package' means all 'package', 'protected' and 'public' methods/fields/classes.

For example, private means all private, package, protected, public. But for this check it will become incorrect description as private will mean only private scope. Maybe scope and exludeScope should become String parameters and declare the name of the scope. Not sure whether we can use 'scope' as a parameter type for this check.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 27, 2016

Member

Are they correct?

yes , that was smth we suppose to create initially.

And also take a look at our description for 'scope' parameter type

#3511 .

Maybe scope and exludeScope should become String parameters and declare the name of the scope.

I think it is good idea.

Member

romani commented Dec 27, 2016

Are they correct?

yes , that was smth we suppose to create initially.

And also take a look at our description for 'scope' parameter type

#3511 .

Maybe scope and exludeScope should become String parameters and declare the name of the scope.

I think it is good idea.

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 12, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 12, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 12, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 13, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 13, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 14, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 14, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 14, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 18, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 18, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 19, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 21, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 21, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 22, 2017

@romani romani removed the bug label Jan 22, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 22, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Jan 22, 2017

romani added a commit that referenced this issue Jan 26, 2017

@romani romani changed the title from ParameterName problem with scope comparison to ParameterName: deprecate 'scope' and 'excludeScope' properties, introduce new property 'accessModifiers' Jan 26, 2017

@romani romani added this to the 7.5 milestone Jan 26, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 26, 2017

Member

fix is merged.

Member

romani commented Jan 26, 2017

fix is merged.

@romani romani closed this Jan 26, 2017

@erikhuizinga

This comment has been minimized.

Show comment
Hide comment
@erikhuizinga

erikhuizinga Jan 26, 2017

This 'fix' causes an error in the event log of IntelliJ IDEA:

The Checkstyle rules file could not be parsed.
cannot initialize module TreeWalker - Property 'accessModifiers' in module ParameterName does not exist, please check the documentation
The file has been blacklisted for 60s.

This happened since less than 30 minutes ago, which leads me to suspect this merge is the cause.

I use google_checks.xml and the Checkstyle-IDEA IntelliJ plugin on IntelliJ IDEA 2016.3.3. The plugin is configured to use this URL as its checkstyle xml source, hence my suspicion this merge caused my checkstyle plugin to error immediately after.

erikhuizinga commented Jan 26, 2017

This 'fix' causes an error in the event log of IntelliJ IDEA:

The Checkstyle rules file could not be parsed.
cannot initialize module TreeWalker - Property 'accessModifiers' in module ParameterName does not exist, please check the documentation
The file has been blacklisted for 60s.

This happened since less than 30 minutes ago, which leads me to suspect this merge is the cause.

I use google_checks.xml and the Checkstyle-IDEA IntelliJ plugin on IntelliJ IDEA 2016.3.3. The plugin is configured to use this URL as its checkstyle xml source, hence my suspicion this merge caused my checkstyle plugin to error immediately after.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jan 26, 2017

Contributor

@erikhuizinga
You gave the incorrect link. I cannot get the file content using it.
https://github.com/checkstyle/checkstyle/issues/blob/master/src/main/resources/google_checks.xml

I assume that you used google_checks.xml from our master branch . That version of google_checks.xml has not been released yet. As you can see this issue has 'breaking compatibility' label which means that after release all changes in this issue become incompatible with previous Checkstyle version.

Please, use google_checks.xml from our previous release (Checkstyle-7.4). Here is a link https://github.com/checkstyle/checkstyle/blob/checkstyle-7.4/src/main/resources/google_checks.xml

Contributor

MEZk commented Jan 26, 2017

@erikhuizinga
You gave the incorrect link. I cannot get the file content using it.
https://github.com/checkstyle/checkstyle/issues/blob/master/src/main/resources/google_checks.xml

I assume that you used google_checks.xml from our master branch . That version of google_checks.xml has not been released yet. As you can see this issue has 'breaking compatibility' label which means that after release all changes in this issue become incompatible with previous Checkstyle version.

Please, use google_checks.xml from our previous release (Checkstyle-7.4). Here is a link https://github.com/checkstyle/checkstyle/blob/checkstyle-7.4/src/main/resources/google_checks.xml

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jan 26, 2017

Contributor

@romani
The link to Checkstyle configuration for 'Google Java Style' refers to master version of google_checks.xml. It means that all changes that we make in google_checks.xml are available for users who want to download the config from Checkstyle site. google_checks.xml usually contains unreleased changes.

The link to Checkstyle configuration for 'Google Java Style' must point to release version of google_checks.xml .

The same is for sun sun_checks.xml. We need to find a way to automate this.

Contributor

MEZk commented Jan 26, 2017

@romani
The link to Checkstyle configuration for 'Google Java Style' refers to master version of google_checks.xml. It means that all changes that we make in google_checks.xml are available for users who want to download the config from Checkstyle site. google_checks.xml usually contains unreleased changes.

The link to Checkstyle configuration for 'Google Java Style' must point to release version of google_checks.xml .

The same is for sun sun_checks.xml. We need to find a way to automate this.

@erikhuizinga

This comment has been minimized.

Show comment
Hide comment
@erikhuizinga

erikhuizinga Jan 26, 2017

@MEZk thanks, my bad! Also, the link was indeed broken, my bad again, but I fixed it.

Is there by chance a permalink to the latest release raw xml? I can only find a URL to the file in the specific release: https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-7.4/src/main/resources/google_checks.xml

@MEZk thanks, my bad! Also, the link was indeed broken, my bad again, but I fixed it.

Is there by chance a permalink to the latest release raw xml? I can only find a URL to the file in the specific release: https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-7.4/src/main/resources/google_checks.xml

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jan 26, 2017

Contributor

@erikhuizinga
Sorry, but now we don't have the link, only specific release. We try to fix this ASAP.

Contributor

MEZk commented Jan 26, 2017

@erikhuizinga
Sorry, but now we don't have the link, only specific release. We try to fix this ASAP.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 26, 2017

Member

s there by chance a permalink to the latest release raw xml?

No. to avoid problems with version differences please use xml config from checkstyle jar, that is embedded.
to reference it do "/google_config.xml" (it will load it from classpath).
Example: http://checkstyle.sourceforge.net/cmdline.html#Download_and_Run

The link to Checkstyle configuration for 'Google Java Style' must point to release version of google_checks.xml .

we need to think how to change search URL to reference strict tag, if that is possible by github API, so we could create issue to make it more secure.

Member

romani commented Jan 26, 2017

s there by chance a permalink to the latest release raw xml?

No. to avoid problems with version differences please use xml config from checkstyle jar, that is embedded.
to reference it do "/google_config.xml" (it will load it from classpath).
Example: http://checkstyle.sourceforge.net/cmdline.html#Download_and_Run

The link to Checkstyle configuration for 'Google Java Style' must point to release version of google_checks.xml .

we need to think how to change search URL to reference strict tag, if that is possible by github API, so we could create issue to make it more secure.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jan 26, 2017

Contributor

@romani
Issue #3760 addresses the problem.

Contributor

MEZk commented Jan 26, 2017

@romani
Issue #3760 addresses the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment