You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by "janniklinde (via GitHub)" <gi...@apache.org> on 2025/12/29 13:02:31 UTC
[PR] Codestyle Enforcing Checks [systemds]
janniklinde opened a new pull request, #2392:
URL: https://github.com/apache/systemds/pull/2392
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "phaniarnab (via GitHub)" <gi...@apache.org>.
phaniarnab commented on PR #2392:
URL: https://github.com/apache/systemds/pull/2392#issuecomment-3698864441
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "janniklinde (via GitHub)" <gi...@apache.org>.
janniklinde commented on PR #2392:
URL: https://github.com/apache/systemds/pull/2392#issuecomment-3697340893
The codestyle check is now an isolated workflow to not fail every other test run
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "mboehm7 (via GitHub)" <gi...@apache.org>.
mboehm7 commented on PR #2392:
URL: https://github.com/apache/systemds/pull/2392#issuecomment-3698781501
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "janniklinde (via GitHub)" <gi...@apache.org>.
janniklinde commented on PR #2392:
URL: https://github.com/apache/systemds/pull/2392#issuecomment-3698840624
> 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, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "phaniarnab (via GitHub)" <gi...@apache.org>.
phaniarnab commented on PR #2392:
URL: https://github.com/apache/systemds/pull/2392#issuecomment-3698810799
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "mboehm7 (via GitHub)" <gi...@apache.org>.
mboehm7 closed pull request #2392: Codestyle Enforcing Checks
URL: https://github.com/apache/systemds/pull/2392
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "janniklinde (via GitHub)" <gi...@apache.org>.
janniklinde commented on PR #2392:
URL: https://github.com/apache/systemds/pull/2392#issuecomment-3697053783
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Codestyle Enforcing Checks [systemds]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2392:
URL: https://github.com/apache/systemds/pull/2392#issuecomment-3697401103
## [Codecov](https://app.codecov.io/gh/apache/systemds/pull/2392?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 71.58%. Comparing base ([`eac7e0e`](https://app.codecov.io/gh/apache/systemds/commit/eac7e0eebaa33d9f306d76c7fe2cc8ab219ffb5c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2834609`](https://app.codecov.io/gh/apache/systemds/commit/2834609fa82b06ab275167bb099be1d187b6adb8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/systemds/pull/2392?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org