checkstyle / eclipse-cs

The Eclipse Checkstyle plug-in integrates the Checkstyle Java code auditor into the Eclipse IDE. The plug-in provides real-time feedback to the user about violations of rules that check for coding style and possible error prone code constructs.
https://checkstyle.org/eclipse-cs
GNU Lesser General Public License v2.1
97 stars 55 forks source link

not understandable sevntu check #570

Open Bananeweizen opened 1 year ago

Bananeweizen commented 1 year ago

In 2 of my PRs I had to disable SimpleAccessorNameNotation. It's raising issues on perfectly valid names which I have checked multiple times. Also the error message and documentation don't really tell anything about how to fix it. I suggest to completely disable it (which will happen anyway if one of those 2 PRs is merged).

rnveach commented 1 year ago

I would report an issue to sevntu if you think it is a bug in the check.

Bananeweizen commented 1 year ago

I'm not sure. I don't know what it is expecting, even after reading documentation. I have a field mSomething, and a getter getSomething(). Not sure if it's confused by the "m" notation (which I don't like myself, but I try to stay consistent with existing code). If you know the check better, you may want to run one of my PRs and remove the disablement locally to see what happens.

rnveach commented 1 year ago

Not sure if it's confused by the "m" notation

I think this is the issue. My understanding of the check, from just refreshing my memory, is (for example) a pure getter method (method that just returns a field) should be named exactly after the field it returns. It should be expecting getMSomething since there is no option to ignore prefixes.

Bananeweizen commented 1 year ago

In that case I would leave the disablement in my PRs and later check if we can do a mass refactoring of the field names to get rid of the m prefixes and to re-enable the check then.

rnveach commented 1 year ago

@Bananeweizen I apologize, SimpleAccessorNameNotationCheck does have a prefix property. However, I believe it only supports one type of prefix, so if eclipse-cs uses multiple then it won't work.

$ cat TestClass.java
public class TestClass {
    private int mSomething;

    void getSomething() { // line 4
      return mSomething;
    }
}
public class TestClassAlt {
    private int mSomething;

    void getMSomething() { // line 11
      return mSomething;
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <property name="severity" value="warning"/>
    <property name="haltOnException" value="false"/>

    <module name="TreeWalker">

<module name="com.github.sevntu.checkstyle.checks.coding.SimpleAccessorNameNotationCheck">
  <property name="prefix" value="m" />
</module>

    </module>
</module>

$ java -jar checkstyle-10.4-sevntu-1.44.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[WARN] TestClass.java:11:5: Unexpected getter name. [SimpleAccessorNameNotation]
Audit done.

If you do remove prefix property, it does behave as I described before.

$ cat TestClass.java
public class TestClass {
    private int mSomething;

    void getSomething() { // line 4
      return mSomething;
    }
}
public class TestClassAlt {
    private int mSomething;

    void getMSomething() { // line 11
      return mSomething;
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <property name="severity" value="warning"/>
    <property name="haltOnException" value="false"/>

    <module name="TreeWalker">

<module name="com.github.sevntu.checkstyle.checks.coding.SimpleAccessorNameNotationCheck">
</module>

    </module>
</module>

$ java -jar checkstyle-10.4-sevntu-1.44.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[WARN] TestClass.java:4:5: Unexpected getter name. [SimpleAccessorNameNotation]
Audit done.