Add regex for line comparison#243
Add regex for line comparison#243cns-solutions-admin wants to merge 5 commits intomathieucarbou:masterfrom
Conversation
This reverts commit 8c6c33b.
|
Also converts a inline header style's name to lower case, otherwise the comparison (and the example in the docs) fails. |
* In maven/xml we need xml:space="preserve" to prevent trimming of text values. * The XML attribute is multiline, not isMultiline. * inline styles are only available in version 4.2 - we should note this, as the suggested version is 4.1. * the inline example does only work, if the inline style name is lowercase. code fixed in previous PR.
|
Hello, |
mathieucarbou
added
in:core
|
Changing the header definition did not work, because the detection of header lines was orginally done by comparing lines with startsWith(beforeEachLine.rtrim()) By introducing a (optional) RegEx for the header lines, this and most other use cases can be covered. |
mathieucarbou
left a comment
mathieucarbou
left a comment
There was a problem hiding this comment.
Please also squash and rebase. Thanks 👍
docs/index.md
Outdated
| <lastLineDetectionPattern>.*\*/(\s|\t)*$</lastLineDetectionPattern> | ||
| <allowBlankLines>false</allowBlankLines> | ||
| <isMultiline>true</isMultiline> | ||
| <multiline>true</multiline> |
There was a problem hiding this comment.
Please remove any change regarding multiline: I've correctly fixed in #247
|
|
||
| private Pattern skipLinePattern; | ||
| private Pattern firstLineDetectionPattern; | ||
| private Pattern otherLineDetectionPattern; |
There was a problem hiding this comment.
I would prefer that the name is called middleLineDetectionPattern
There was a problem hiding this comment.
Except it is for all lines except the first line - the last line must also pass this pattern (or the beforeEachLine test).
Finding the best name is often the most difficult task ;-)
There was a problem hiding this comment.
the last line must also pass this pattern
Why ? There is a pattern for the last line.
There was a problem hiding this comment.
But the code still checks beforeEachLine. I didn't want to change the logic and the RegEx happens when the startsWith(beforeEachLine) check happens.
| check("endLine", this.endLine); | ||
| check("afterEachLine", this.afterEachLine); | ||
| check("firstLineDetectionPattern", this.firstLineDetectionPattern); | ||
| // other line detection pattern can be null |
There was a problem hiding this comment.
no need for this comment: it's like beforeEachLine ;-)
| final HeaderDefinition headerDefinition = new HeaderDefinition( | ||
| "javadoc2_style", "/**", " ** ", " **/", "", null, "^\\s*/\\*.*$", "^\\s*\\*.*$", "^.*\\*/\\s*$", false, true, false); | ||
| return new HeaderParser(new FileContent(new File(fileName), System.getProperty("file.encoding")), headerDefinition, new String[]{"copyright"}); | ||
| } |
There was a problem hiding this comment.
You would need also to have a more complete test (like CompleteMojoTest does) to also test the other plugin actions, like really testing the issue you want to fix.
| this.afterEachLine = afterEachLine; | ||
| this.skipLinePattern = compile(skipLinePattern); | ||
| this.firstLineDetectionPattern = compile(firstLineDetectionPattern); | ||
| this.otherLineDetectionPattern = compile(otherLineDetectionPattern); |
There was a problem hiding this comment.
you would need to update setPropertyFromString(...) too for xml header definition.
mathieucarbou
added
is:bug
There was a problem hiding this comment.
Pull Request Overview
Adds support for a customizable regex to detect header lines beyond the first in multi-line comments, along with sample files, tests, API changes, and documentation updates.
- Introduced
otherLineDetectionPatterninHeaderDefinitionandHeaderStyle - Switched
HeaderParserto useisOtherHeaderLine(...)instead of a fixed prefix check - Added three Java header fixtures and corresponding parser tests, plus whitespace‐preservation updates in docs
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| license-maven-plugin/src/test/resources/doc/java1.txt | New sample header with two‐star prefix |
| license-maven-plugin/src/test/resources/doc/java2.txt | New sample header with single‐star prefix |
| license-maven-plugin/src/test/resources/doc/java3.txt | New sample header without space after slash |
| license-maven-plugin/src/test/java/com/mycila/maven/plugin/license/header/HeaderParserTest.java | Added tests for the three Java header samples |
| license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/header/HeaderParser.java | Replaced hardcoded startsWith(before) with isOtherHeaderLine |
| license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/header/HeaderDefinition.java | Added otherLineDetectionPattern, new constructor, and isOtherHeaderLine |
| license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/HeaderStyle.java | Exposed otherLineDetectionPattern as a plugin parameter |
| docs/index.md | Added xml:space="preserve", marked version, but docs lack new pattern |
Comments suppressed due to low confidence (2)
docs/index.md:545
- The
<javadoc_style>snippet doesn’t include the new<otherLineDetectionPattern>element. Please add an example of this setting to document the new optional regex.
<firstLineDetectionPattern>(\s|\t)*/\*.*$</firstLineDetectionPattern>
license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/header/HeaderDefinition.java:67
- The variable
isMultilineis not defined in this constructor scope; it should bemultiLineto match the parameter name.
this(type, firstLine, beforeEachLine, endLine, afterEachLine, skipLinePattern, firstLineDetectionPattern, null, lastLineDetectionPattern, allowBlankLines, isMultiline, padLines);
This solves #242 by adding a new optional regex to compare the lines other than the first for multiline headers.