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

Introduce the LeftCurlyCheck with a new message #12564

Open
Kevin222004 opened this issue Dec 31, 2022 · 17 comments
Open

Introduce the LeftCurlyCheck with a new message #12564

Kevin222004 opened this issue Dec 31, 2022 · 17 comments

Comments

@Kevin222004
Copy link
Contributor

Kevin222004 commented Dec 31, 2022

I have read check documentation:
https://checkstyle.org/config_blocks.html#LeftCurly
I have used latest version of checkstyle https://github.com/checkstyle/checkstyle/releases/download/checkstyle-10.6.0/checkstyle-10.6.0-all.jar
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

$ cat Test.java
class Test {

    void summation()
    /* comment */ { } // violation

    void multiplication()
    { // violation 
    }

 }

$ cat config.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"/>

    <module name="TreeWalker">
        <module name="LeftCurly">
         <property name="option" value="eol" />
    </module>
    </module>

</module>

output :- 
kevin@inspiron-15-5510:~/Desktop/check_style$ java -jar checkstyle-10.6.0-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /home/kevin/Desktop/check_style/Test.java:7:5: '{' at column 5 should be on the previous line. [LeftCurly]
Audit done.
Checkstyle ends with 1 errors.

we can make violation like // violation '{' is expected at the end of line so user can decide it where to put
the LeftCurly

  1. put '{' on previous line
  2. move } to next line

more information #12548 (comment)

Attention: we can introduce new message and migrate to it in few iterations. If current message is used good in some context, we should not change it.

@nrmancuso
Copy link
Member

@Kevin222004 please follow issue template.

@Kevin222004
Copy link
Contributor Author

@nrmancuso updated

@romani
Copy link
Member

romani commented Jan 2, 2023

@Kevin222004 , please use officially released version , you used checkstyle-10.6.0-SNAPSHOT.
this issue should not be dependent on issue to make Check comments aware.

@Kevin222004
Copy link
Contributor Author

@romani done

@romani
Copy link
Member

romani commented Jan 2, 2023

please update Test.java to have violation.

@Kevin222004
Copy link
Contributor Author

please update Test.java to have violation.

is it fine

@romani
Copy link
Member

romani commented Jan 2, 2023

Please remove method it confuses.

@Kevin222004
Copy link
Contributor Author

done

@Kevin222004
Copy link
Contributor Author

@romani ping

@romani
Copy link
Member

romani commented Jan 8, 2023

All // violation should match to violation in flu output. So there should be single comment // violation

@Kevin222004
Copy link
Contributor Author

@romani ping

@romani
Copy link
Member

romani commented Jan 16, 2023

please remove from /* comment */ { } // violation trailing comment.
there is no violation here.

remove summation it is not required at all.


for:

void multiplication()
    { // violation 
    }

Test.java:7:5: '{' at column 5 should be on the previous line.

{ is technically already at end of line and it is alone. So violation is actually correct.
So we are confused again.

@romani
Copy link
Member

romani commented Jan 16, 2023

@Kevin222004 , do you have any recommendations for doc update
at https://checkstyle.org/property_types.html#LeftCurlyOption

Option Definition
eol The brace must always be on the end of the line. For example:# if (condition) {

to make clear that "end of line" means something like "same line as block parent (identifier/expression/declaration)"

@Kevin222004
Copy link
Contributor Author

Kevin222004 commented Jan 17, 2023

{ is technically already at end of the line and it is alone. So violation is actually correct.
So we are confused again.

I am also very confused now about this issue 🙃. that which msg is good. where to show which msg end of line or previous line.

if I rethink this from the beginning,
currently, code(logic) will check whether there is whitespace before { or not.

public static boolean hasWhitespaceBefore(int index, String line) {

know maybe those who have written that code thought about the logic that there is no code possible between { and
identifier/expression/declaration, in any case, there will be no code there so if { would trail with whitespace then
it is obvious that it is on a new line so we need it on previous line to put { at end of the line where it will be as same as block parent. where the message is looking very good that { should be on the previous line. this is the current logic and situation.

the idea of the new message has begun from #12548 (comment)
from the changes

    void method1 ()
    /* violation not reported */ { } // violation ''{' at column 34 should be on the previous line'

know this is also very weird code to put comment like this if we want to put it { at end of line as same as parent block then i think the message is looking very good `{` is expected at end of line
according to me this is only the situation where we need this new message and i think this will also not violate the google java style rule which checkstyle follow

lastly we can change the entire message should be on previous line as end of line . we also have to change the doc
https://checkstyle.org/config_blocks.html#LeftCurly and https://checkstyle.org/property_types.html#LeftCurlyOption
in this case to explain the user and contributor that what exactly end of line means

@Kevin222004
Copy link
Contributor Author

Kevin222004 commented Jan 17, 2023

@Kevin222004 , do you have any recommendations for doc update at https://checkstyle.org/property_types.html#LeftCurlyOption

Option Definition
eol The brace must always be on the end of the line. For example:# if (condition) {
to make clear that "end of line" means something like "same line as block parent (identifier/expression/declaration)"

yes we have to update this to make it very clear the meaning of end of line and if this pr is approved then https://checkstyle.org/config_blocks.html#LeftCurly we also need to update this to make it very clear the meaning of message end of line or i think in general we have to update it to make the meaning of eol clear

@Kevin222004
Copy link
Contributor Author

@romani what is your opinion

@Kevin222004
Copy link
Contributor Author

@romani should i create the minor pr to update it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants