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

RightCurly with option alone false negative for class, method and constructor #6345

Closed
robertpainsi opened this issue Jan 4, 2019 · 5 comments

Comments

Projects
None yet
5 participants
@robertpainsi
Copy link
Contributor

commented Jan 4, 2019

/var/tmp $ javac MyClass.java

/var/tmp $ 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"/>
    <module name="TreeWalker">
        <module name="RightCurly">
            <property name="option" value="alone"/>
            <property name="tokens" value="CLASS_DEF, METHOD_DEF, CTOR_DEF"/>
        </module>
    </module>
</module>

/var/tmp $ cat MyClass.java
public final class MyClass {
    public MyClass() {} // Doesn't warn - incorrect
    
    public void main() {} // Doesn't warn - incorrect
    
    class Foo {} // Doesn't warn - incorrect
}

/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-8.16-all.jar -c config.xml MyClass.java
Starting audit...
Audit done.

In all three cases (constructor, method, class), the right curly brace is on the same line as the left curly brace. However, the option alone should force the right curly brace to be on it's own line,

Correct:

public final class MyClass {
    public MyClass() {
    }
    
    public void main() {
    }
    
    class Foo {
    }
}

https://groups.google.com/forum/#!topic/checkstyle/q_kVS42ITt0

@romani romani added the approved label Jan 5, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 4, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 4, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 5, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 5, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 5, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 5, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 6, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 6, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 6, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 26, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Mar 31, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Apr 1, 2019

fzdy1914 added a commit to fzdy1914/checkstyle that referenced this issue Apr 1, 2019

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

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

@rnveach rnveach closed this in #6505 Apr 5, 2019

rnveach added a commit that referenced this issue Apr 5, 2019

@rnveach rnveach added this to the 8.20 milestone Apr 5, 2019

@romani romani added the bug label Apr 6, 2019

@Vampire

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Was this really necessary while we try to get #6565 finished? grml

Besides that, is this really intended, because as mentioned at #3775 (comment), this behavior was explicitly added as fixing a false-positive for #213 while now the behavior is again reverted as false-positive. This feels a bit like ping pong. And unfortunatley my question in #3775 didn't get answered.

@rnveach

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Sorry but when mutliple people work on a project, conflicts are bound to happen. This issue was started first and got through the review process first so it was merged first.

I am also waiting answers to questions and PRs but @romani just gets too busy and is unfortunately is the center for alot of design decisions.

really intended, because as mentioned at #3775 (comment), this behavior was explicitly added as fixing a false-positive for #213 while now the behavior is again reverted as false-positive

We did discuss this in PR at #6505 (comment) and @romani said it was a mistake to affect alone option. I did not go back to the right issue when pointing it out but I believe he will still considered the original change wrong.

If anything, this check is too complicated with a single option that tries to encompass everyone's needs. I would prefer it get re-implemented with something like #4086 .

@Vampire

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Sorry but when mutliple people work on a project, conflicts are bound to happen. This issue was started first and got through the review process first so it was merged first.

But you could at least have mentioned that there is some other PR for rcurly going on, then I would have been able to base my work on it or incorporate it or whatever.
Now I have merge conflicts and various failing tests, when we almost finished this thing already.

We did discuss this in PR at #6505 (comment) and @romani said it was a mistake to affect alone option.

But the discussion had wrong assumptions.
This line was not added accidentally for alone_or_singleline, but intentionally for alone as I posted and linked above.
So the statement "it was a mistake to affect alone option" is absolutely meaningless in this context as it was no accidental change, but a much older intentional change that was approved by @romani himself.
Now there are two issues, #213 and this one that state the exact opposite as false-positive and both were accepted, so we play ping pong.

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 27, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 27, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 28, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 28, 2019

@Vantuz

This comment has been minimized.

Copy link

commented Apr 29, 2019

Is there a way to set up this check so it would allow single line empty braces, but force right curly brace to be alone on the line in case block is not empty. I want following behaviour:

public final class MyClass {
    public MyClass() {} // Should not warn
    
    public void main() {} // Should not warn
    
    public void test1() {
        doSomething();
    } // Should not warn

    public void test2() {} // Should not warn

    public void test3() {
    } // Should warn

    public void test4() { doSomething(); } // Should warn
}
@rnveach

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@Vantuz
Your best bet is to report a new issue. Nothing supports what you want for test3. For things like test2 you can use alone_or_singleline but then it won't produce an exception for test4.

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.