Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

build: Add OperatorWrap rule to checkstyle #620

Closed
Oliver-Loeffler opened this issue Mar 24, 2023 · 4 comments · Fixed by #621, #625 or #626
Closed

build: Add OperatorWrap rule to checkstyle #620

Oliver-Loeffler opened this issue Mar 24, 2023 · 4 comments · Fixed by #621, #625 or #626
Labels
enhancement New feature or request

Comments

@Oliver-Loeffler
Copy link
Collaborator

Oliver-Loeffler commented Mar 24, 2023

As learned with issue-73, operators are supposed to be wrapped into spaces.
Nicely Checkstyle has a rule for that.

Expected Behavior

Unwrapped operators such as in content=textArea.getText(); should be detected by Checkstyle.
Checkstyle should issue a warning for such cases. Ideally the source looks like: content = textArea.getText();.

Current Behavior

Checkstyle does not detect those, as it does not check for this pattern.

Context

Inconvenience / extra effort in Pull Request reviews. Would be good if we could avoid this by using Checkstyle.

I'll open a PR with a modified checkstyle.xml.
If there other rules we should implement, we can do this in the related PR.

@Oliver-Loeffler
Copy link
Collaborator Author

Actually I was wrong with this one. Just played around with Checkstyle and found that OperatorWrap only ensures that we get the following pattern when line wraps are needed:

Checkstyle will complain this one:

        int d = c +
                10;

It will permit:

        int d = c
                + 10;

Nevertheless, this rule is okay to have. What I actually wanted (I am sorry, did not check carefully enough) is the following:

        <module name="WhitespaceAfter"/>
        <module name="WhitespaceAround"/>

This setup ensures that we actually get the whitespaces around lambdas, around operators +, ||, etc.
Interestingly this also ensures that we have actually no whitespace before comma but a following whitespace after the comma.
I'll test my PRs using those settings and rework them accordingly.

If you agree @abhinayagarwal, I'll issue an updated PR referring this issue.

@Oliver-Loeffler
Copy link
Collaborator Author

One more question @abhinayagarwal , should we apply Checkstyle also to the test sources?

@abhinayagarwal
Copy link
Collaborator

My preference would be for all java sources to have these checkstyles.

@Oliver-Loeffler
Copy link
Collaborator Author

Oliver-Loeffler commented Mar 28, 2023 via email

# for free to join this conversation on GitHub. Already have an account? # to comment