-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #269 Keep comments in .flattened-pom.xml #270
Conversation
/** | ||
* The unique path list for an original node (the comments are stored via the referenced previousSibling) | ||
*/ | ||
private Map<String,AtomicReference<Node>> commentsPaths; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why AtomicReference? I think this is used from a single thread only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, AtomicReference is more for threading.
When taking java.util.Optional as replacement it would not really be optional (as BiConsumer would never get an empty one). I will check.
Correction: "as BiConsumer would never get an empty one" => can be null when new elements are added during flatten.
So I take Optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I take Optional.
Value of Optional cannot be set later, so I need to take something other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to pass the nodePath to the BiConsumer directly.
* The core maven model readers/writers are discarding the comments of the pom.xml. | ||
* By setting keepCommentsInPom to true the current comments are moved to the flattened pom.xml. | ||
*/ | ||
@Parameter( property = "flatten.dependency.keepComments", required = false ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could say what the default value is (false).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added defaultValue = "false" and provided the info in the javadoc
( commit 522b1a4 )
In the test runs I see that JDK 11 is writing different sequence of attributes, https://github.com/mojohaus/flatten-maven-plugin/runs/5890341535?check_suite_focus=true |
Added jdk11 specific expected result. (different properties sequence) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow code style conventions https://maven.apache.org/developers/conventions/code.html
especially for new files ...
Please also squash to one final commit |
94d6d09
to
0651783
Compare
Done. |
80aab0e
to
b496e96
Compare
See #269