-
Notifications
You must be signed in to change notification settings - Fork 866
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
Consider removing or loosening DeclarationOrder checkstyle #1880
Comments
Hi @anuraaga, we understand the motivation for code consistency but we don't see a problem in the current checkstyle, and changing it would impact Java SDK code. |
I'll give one example I found in a quick search in this repo Line 33 in cdfb7d3
There's a not-useful method above all the important ones, and most codebases I've seen would try to ensure methods are ordered in terms of the importance to a user of that class. If this codebase doesn't find it to be an issue, it's fine, but I'd definitely recommend another thought. |
@anuraaga We don't see this as a big issue right now, but we appreciate you reaching out to us. Consider changing the checkstyle in the X-Ray SDK project, even if it breaks the consistency between the two repos. |
…0ba17fee9 Pull request: release <- staging/51e464f1-5861-4476-84bd-52e0ba17fee9
Describe the issue
Currently checkstyle applies the declarationorder rule which requires all constructors to go above methods, including static methods. This results in weird code when declaring static factories and private constructors. For example here,
https://github.com/anuraaga/aws-xray-sdk-java/blob/61fedbfaa2040c78a5dd41f1e40a2dd7237bb192/aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/entities/TraceID.java
I think the sources are more useful to users if the entry point to a class is at the top and for ones that use factories, that includes static methods. Does it make sense to tweak or remove this check to allow that?
Note, I added the checkstyle from here to the x-ray SDK to encourage consistency between the two repos. Consistency is more important to me so I would stick with the current behavior if we can't change the checkstyle here.
The text was updated successfully, but these errors were encountered: