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

checkstyle RightCurly not correct when there are comments #637

Open
xenoterracide opened this issue Jan 16, 2024 · 6 comments
Open

checkstyle RightCurly not correct when there are comments #637

xenoterracide opened this issue Jan 16, 2024 · 6 comments

Comments

@xenoterracide
Copy link

xenoterracide commented Jan 16, 2024

/CustomerAuditServiceImpl.java:441:9: '}' at column 9 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurly]

**Prettier-Java 2.5.0 **

Input:

I apologize, this hot mess is so long, but I'm wondering if that's part of what's causing the problem. I added a violation in the code here.

        if (
            isAddNode &&
            (NODE_CARD.equalsIgnoreCase(nodeLabel) ||
                NODE_PERSON.equalsIgnoreCase(nodeLabel) ||
                NODE_ADDRESS.equalsIgnoreCase(nodeLabel) ||
                NODE_PHONE.equalsIgnoreCase(nodeLabel) ||
                NODE_HOUSEHOLD.equalsIgnoreCase(nodeLabel))
        ) {
        } else if (isNodePropChange && !StringUtils.isEmpty(getPropertiesAsString(audit))) {
            String propString = getPropertiesAsString(audit);
            String keyNumber = ignoreUuids(primaryKey);
            responseString = String.format("Change to POSID: %s - %s", keyNumber, propString);
        }
        // Attributes (which are relations)
        else if (isAddRelation && isAttributeType) { // ** VIOLATION **
            // TX Template: SetLoyaltyAttribute_A
            // Seems like this will also handle Un/Enroll Customer and NationalRewards
            String attrName = getAttributeName(audit);
            responseString = String.format("Attribute %s added with value %s", attrName, propValue);
        } else if (isChangeRelation && isAttributeType) {

Expected behavior:

...

I'm not sure this is wrong per prettier, but I don't see a way to fix per checkstyle either. Please leave open until one or the other can be resolved.

@jtkiesel
Copy link
Contributor

Prettier JavaScript keeps the comment in this position (example), so I don't think we will change the formatting of this. We could potentially update the Checkstyle configuration that we provide.

@jtkiesel
Copy link
Contributor

jtkiesel commented Jul 28, 2024

I'm leaning towards simply removing the Checkstyle configuration that this repo provides. We haven't been maintaining it, and IMO there isn't really a good reason for Prettier Java to provide a Checkstyle configuration. If you use Checkstyle and Prettier Java, I would imagine you would want to omit/disable any Checkstyle rules that duplicate responsibilities of Prettier Java. At that point, anything left in the Checkstyle ruleset wouldn't have anything to do with Prettier Java, and would just be personal preference.

@xenoterracide As it seems you've been using the provided Checkstyle configuration, I'm curious what your thoughts are on the above? Is there a valid use-case for a Checkstyle configuration that matches Prettier Java's style that I'm not considering?

@xenoterracide
Copy link
Author

xenoterracide commented Jul 28, 2024

There are many reasons to use check style in addition to prettier... However let's focus on purely code formatting rules that prettier could cover. Prettier including prettier Java does not comprehensively cover even code files. There are many rules that could be argued though that prettier does cover like this one.

Arguably the big reason is not to have to run node in CI on every build. The only node job I have in CI is to update node dependencies. To validate that developers are using prettier's style you would have to install node and the prettier dependencies and run the check in CI.

@xenoterracide
Copy link
Author

Also, one benefit although imperfect... Is that it could theoretically catch a regression in prettier-java. However I have found that check style cannot validate all of What prettier does. In fact I had one of their developers downright insult The style because it wasn't one Three they had seen before... As it's not that yet common to use in the Java world. I would argue though it's the only one that is sensible for lambda programming...

@jtkiesel
Copy link
Contributor

jtkiesel commented Jul 28, 2024

Sorry, I'm not sure how I messed up that user mention, I meant to direct that question to you.

To clarify my question, I'm not asking why one would want to use Checkstyle and Prettier together. There are a number of code-style-related things that Prettier doesn't touch, and so for things like Javadoc style, class/variable/method naming style, etc. it makes sense to use another tool such as Checkstyle.

However, it sounds like you're saying you require your devs to install Node.js and Prettier to format code, but don't want to have to do so on CI to validate the code is formatted? There are quite a lot of code style choices that Prettier makes that I don't believe have equivalent Checkstyle rules, so while Checkstyle may work for some teams using Prettier Java, I think our primary recommendation should be for projects to use prettier --check for validation.

I don't know that it makes sense for us to provide and maintain these Checkstyle rules, given that we're developing a Prettier plugin, and Prettier has its own means of validating code style.

@xenoterracide
Copy link
Author

Yes that's what I'm saying. Yes you are correct as far as the fact that prettier does a lot of things that they don't and can't do and vice versa.

You could take the rules (ideally as corrected from any bugs I've opened) And open a bug over there and ask if they will maintain them like they do the Google and Sun rules. Maybe they would be willing to host a copy.

An additional benefit that I don't personally care much about is that some people have check style validation enabled in their IDE. Obviously they should have prettier valid formatting enabled too but it would highlight The errors if Auto formatting was broken. I've had IntelliJ fail at auto formatting so many times due to lack of ease of use of prettier. It simply does not set up node And prettier correctly by default. It doesn't set up checkstyle by default though either. So in both cases whenever I set up a new project I have to go through and correct all of its nonsense.

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

2 participants