Repository navigation
[#26] Enforce formatting - #51
MohammedNaser28 wants to merge 1 commit into
Conversation
|
@jesperpedersen if this config and rules accepted i can do it as a separate commit in this PR, or a separate PR? |
a10b283 to
793a642
Compare
|
@MohammedNaser28 Do the formatting as well |
793a642 to
6f75412
Compare
|
@MohammedNaser28 Sorry rebase is needed now |
|
@sksingh2005 no problem i will rebase |
6f75412 to
3135444
Compare
|
@sksingh2005 Done |
sksingh2005
left a comment
There was a problem hiding this comment.
Thanks for this! The tooling does what #26 asked for: both the backend and frontend are formatted, Java uses google-java-format with 2-space indents, and the checks run in CI.
What I verified on the branch:
- CI is green (
testandfrontend-check). mvn spotless:check checkstyle:check test-compilepasses locally.- Frontend lint has 0 errors and the vitest tests pass (4/4).
- Checkstyle blocks bad code: I added an unused import and the build failed with
UnusedImports. - The reformat changes only layout. In the Java files, only whitespace and Javadoc wrapping changed, imports are unchanged, and no string literals were split.
There are some things to address before merging.
Required changes
-
The description and comments don't match the config.
The PR body says CI checks "changed files only (ratchet against main)", and theMakefilecomment says "Reformats Java changed files". There is noratchetFromin the Spotless config, so it checks all files. That's correct, since the whole codebase was reformatted, but please update the PR description, theMakefilecomments,CONTRIBUTING.mdanddocs/BUILDING.mdto say that. -
The Checkstyle rules don't match the description.
The PR body saysOneStatementPerLineis not enabled, butcheckstyle.xmlenables it. The XML comment lists onlyNeedBracesandAvoidStarImportas deferred. Please make both agree with what is actually configured.
Suggested changes
-
Line endings on Windows. With
core.autocrlf=true,npm run format:checkreports 226 files, all because of CRLF line endings. Please add"endOfLine": "auto"to.prettierrc.json, or add* text=auto eol=lfto.gitattributes. -
Fail fast in CI. In
ci.yml, move thespotless:checkandcheckstyle:checksteps beforemvn test, so formatting failures show up without waiting for the full test run. -
Hook the checks into the normal flow.
mvn verifyandmake testdon't run the formatting checks right now. Please bindspotless:checkandcheckstyle:checkto theverifyphase, or callformat-checkfrommake test. -
Prettier coverage. The globs only match
src/**, soindex.html,eslint.config.js,vite.config.jsandvitest.config.jsare not checked, andindex.htmlalready fails Prettier. Please widen the globs, or say in the docs that these files are excluded on purpose. -
Small fix: remove the trailing whitespace in the step name
"Check formatting "in bothci.ymlandlint.yml.
Follow-up (not blocking)
- Please open an issue to enable
NeedBracesandAvoidStarImportonce the existing violations are fixed. - This PR touches almost every source file, so other open PRs will need to rebase and run
make formatafter it's merged.
Thanks again, nice work overall!
|
We are using Unix line endings |
Add make format, format-check and checkstyle targets (Spotless with google-java-format, Prettier, Checkstyle) and run them in CI. Format the existing code. Add my name to the acknowledgement file.
3135444 to
9cbb9fe
Compare
|
@sksingh2005 @jesperpedersen |
Close #26
This PR adds
make format,make format-checkandmake checkstyle, and runs them in CI.CI checks formatting of all files, and the existing code is formatted in this PR.
make testruns them too..gitattributes.src/frontendfolder, soindex.htmland the config files are formatted too.Checkstyle enforces unused/redundant imports, tabs and OneStatementPerLine;
NeedBraces and AvoidStarImport would fail on many places in existing code, so they are not enabled yet.
Other open PRs need to rebase and run
make formatafter this is merged.