Codestyle Enforcing Checks

[janniklinde]

This patch aims to improve codestyle consistency for future contributions.

There are two main issues with contributions to SystemDS:

1. Some codestyles are enforced, but there is no automatic check to enforce that style before manual review (e.g., unused imports, wildcard imports, tabs vs spaces)
2. Contributions introduce inconsistent code styles (e.g., variable naming; private int _x or private int x, or spacing in control flow)

To improve consistency, a checkstyle.xml was added as a build plugin. This file contains mandatory codestyle checks such as:

• Required leading tabs and not spaces (except in comments)
• Unused imports detection
• Wildcard imports detection
• Some naming conventions (e.g., type names)

To allow incremental codestyle improvements, existing failing codestyle checks are suppressed in suppressions.xml.

While some naming conventions are already enforced in this patch (such as method name and type names), some have been disabled because there are no clear guidelines and must yet be discussed. I would particularly be interested in leading underscores in member variables, which currently differ from class to class and are sometimes inconsistent even within the same class.

[janniklinde]

My codestyle tests seemed to fail due to a recent merge of einsum, which violates quite a lot of these checks. I will do a rebase and exclude these files for now

[janniklinde]

The codestyle check is now an isolated workflow to not fail every other test run

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.58%. Comparing base (eac7e0e) to head (2834609).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2392      +/-   ##
============================================
- Coverage     71.58%   71.58%   -0.01%     
- Complexity    47152    47155       +3     
============================================
  Files          1531     1531              
  Lines        181131   181131              
  Branches      35582    35582              
============================================
- Hits         129670   129663       -7     
- Misses        41578    41584       +6     
- Partials       9883     9884       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mboehm7]

LGTM - thanks @janniklinde for the initiative and patch. I think this is a great idea, and sorry that I missed a few of these instances on recent merges. I'll clean some of these up in the next days.

[phaniarnab]

This is indeed great @janniklinde. It will make it easier to merge smaller patches from external contributors.
Wondering is there a way to check the codestyle against the dev/CodeStyle_eclipse.xml file? There are many other checks available in that file, such as line length.

[janniklinde]

This is indeed great @janniklinde. It will make it easier to merge smaller patches from external contributors. Wondering is there a way to check the codestyle against the dev/CodeStyle_eclipse.xml file? There are many other checks available in that file, such as line length.

Should be possible @phaniarnab, I can look into that. However, I guess that we will have to suppress a lot of existing java classes because these checks will be quite strict. In general, I agree that it should be enforced to run the code formatter before merging to keep the styles consistent, I am just not sure how to cleanly transition to that state.

[phaniarnab]

This is indeed great @janniklinde. It will make it easier to merge smaller patches from external contributors. Wondering is there a way to check the codestyle against the dev/CodeStyle_eclipse.xml file? There are many other checks available in that file, such as line length.

Should be possible @phaniarnab, I can look into that. However, I guess that we will have to suppress a lot of existing java classes because these checks will be quite strict. In general, I agree that it should be enforced to run the code formatter before merging to keep the styles consistent, I am just not sure how to cleanly transition to that state.

Thanks. I do not have a clear answer. I think your approach of incrementally adding new rules is likely the right approach. Our goal is to enforce consistent code style for developers, without requiring them to modify or reformat existing code written by others.