STORM-8585: Sync checkstyle config with upstream google_checks.xml - #8969
STORM-8585: Sync checkstyle config with upstream google_checks.xml#8969sercuzz8 wants to merge 5 commits into
Conversation
Same fix as the earlier BoxComments cleanup: replace box-border comments with plain single-line comments in files that hadn't been touched yet.
61c9bd5 to
99ce17f
Compare
|
@Gianluca Graziadei You started this one: #8585 |
| extrajars=[USER_CONF_DIR, STORM_BIN_DIR]) | ||
|
|
||
|
|
||
| def merge_args_preserving_order(sys_args, known_args, unknown_args): |
There was a problem hiding this comment.
this does not pertain checkstyle changes. What do you think about putting it in a separate PR?
There was a problem hiding this comment.
I did this just because not applying this fix blocked the execution of mvn validate and mvn clean install but I agree with this idea.
Just to be sure, can I use the same issue to propose this?
In any case, thank you for the attention and the advice.
|
Hi @sercuzz8, thanks for your effort and for driving this initiative!
That said, fixing these rules manually module by module can become quite tedious. I want to make sure your time and effort deliver the highest possible value to both you and the project. Either way, it's your call. Once we resolve the module warnings, we can consider aligning our checkstyle rules with the upstream Google manifest. When you're ready for that step, please open a proposal thread on dev@ first so we can gather community consensus. What do you think? |
|
Hello @GGraziadei, |
|
I agree with @reiabreu about the python change, besides that LGTM |
4 participants
Purpose
Update
storm_checkstyle.xmlto follow Google conventionsgoogle_checks.xmlThere is no strong rationale but I saw it was originally used as a base.
Changes:
OperatorWrapintoOperatorWrapNL/OperatorWrapEOLso lambdas get upstream's dedicated EOL wrap rule.BoxCommentsvariant ofTodoComment, catching box-like repetitive comment banners (e.g.// ====...).IndentationChecksuppression regex to upstream's anchored form.Not in Google checkstyle:
NewlineAtEndOfFileto follow POSIX's line definition.Tests and Implementation
mvn -pl storm-checkstyle install -DskipTeststo rebuild the shared ruleset artifact.mvn validateacross the full reactor (OK withBUILD SUCCESSor0 Checkstyle violations/ KO warranted code correction on interested lines until OK).