SONARJAVA-6196 S1451 should provide a default template for headers#5578
SONARJAVA-6196 S1451 should provide a default template for headers#5578NoemieBenard merged 4 commits intomasterfrom
Conversation
SummaryThis PR adds a meaningful default template for the What reviewers should knowReview focus: This is a straightforward constant update to the rule's default behavior. The change affects how the rule behaves when enabled without explicit configuration. Key points:
Testing to verify: Confirm that rules using this default now flag files that don't match the template pattern (regression test), and that customized headers still work as expected.
|
| check = new FileHeaderCheck(); | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/FileHeaderCheck/Class6.java")) | ||
| .withCheck(check) | ||
| .verifyNoIssues(); |
There was a problem hiding this comment.
The test verifies that Class6.java (which contains the default header) is compliant — but there is no negative test asserting that a file without the default header gets flagged.
This gap matters concretely: if DEFAULT_HEADER_FORMAT ever became empty (e.g., due to a text-block indentation accident), the headerFormat.isEmpty() branch would fire, set expectedLines = {}, and matches() would return true for any file. The verifyNoIssues() call would still pass, giving a false sense of correctness.
Add a complementary case that runs the default FileHeaderCheck() against a file that lacks the header and asserts an issue is raised — something like:
check = new FileHeaderCheck();
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/FileHeaderCheck/Class1.java"))
.withCheck(check)
.verifyIssueOnFile("Add or update the header of this file.");- Mark as noise
There was a problem hiding this comment.
Let's use desciprive file names for test samples.
| private String getHeaderFormat() { | ||
| String format = headerFormat; | ||
| if(format.charAt(0) != '^') { | ||
| if (format.charAt(0) != '^') { |
There was a problem hiding this comment.
IMO it's a bad practice to do unintentional chages, because they make it hard to understand what is happening, if someone is reading a PR for some reason in the future.
| .onFile(mainCodeSourcesPath("checks/FileHeaderCheck/Class3.java")) | ||
| .withCheck(check) | ||
| .verifyNoIssues(); | ||
|
|
There was a problem hiding this comment.
Are these a part of this PR or is it a copied from the other changes?
There was a problem hiding this comment.
Let's remove expectedLines.
There was a problem hiding this comment.
Let's remove this method.
There was a problem hiding this comment.
This should be extracted to checkExpectedLines
dfabda9 to
b5eee2c
Compare
|
There was a problem hiding this comment.
LGTM! ✅
The text block is correct — the 4 spaces of incidental whitespace strip cleanly to produce the expected header, matching ClassDefaultHeader.java exactly. The @RuleProperty(defaultValue = DEFAULT_HEADER_FORMAT) annotation compiles fine since text blocks are compile-time constants. The previously flagged missing negative test has not been addressed.
tomasz-tylenda-sonarsource
left a comment
There was a problem hiding this comment.
LGTM. I think we should take a look at the experience in local SQ to see how changing this rule will affect the users.





Add default template for headerFormat in S1451.