Skip to content
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

Merged
merged 1 commit into from Aug 15, 2022

Conversation

ca-stefan-cordes
Copy link
Contributor

See #269

/**
* The unique path list for an original node (the comments are stored via the referenced previousSibling)
*/
private Map<String,AtomicReference<Node>> commentsPaths;
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 )
Copy link
Collaborator

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).

Copy link
Contributor Author

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 )

@ca-stefan-cordes
Copy link
Contributor Author

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
so I need to adapt assert in KeepCommentsInPomTest

@ca-stefan-cordes
Copy link
Contributor Author

Added jdk11 specific expected result. (different properties sequence)

ca-stefan-cordes added a commit to c-a-services/flatten-maven-plugin that referenced this pull request May 3, 2022
Copy link
Member

@slawekjaranowski slawekjaranowski left a 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 ...

@slawekjaranowski
Copy link
Member

Please also squash to one final commit

@slawekjaranowski slawekjaranowski linked an issue Aug 7, 2022 that may be closed by this pull request
ca-stefan-cordes added a commit to c-a-services/flatten-maven-plugin that referenced this pull request Aug 8, 2022
ca-stefan-cordes added a commit to c-a-services/flatten-maven-plugin that referenced this pull request Aug 8, 2022
ca-stefan-cordes added a commit to c-a-services/flatten-maven-plugin that referenced this pull request Aug 8, 2022
ca-stefan-cordes added a commit to c-a-services/flatten-maven-plugin that referenced this pull request Aug 8, 2022
@ca-stefan-cordes
Copy link
Contributor Author

Please also squash to one final commit

Done.

@slawekjaranowski slawekjaranowski merged commit a940cb1 into mojohaus:master Aug 15, 2022
@ca-stefan-cordes ca-stefan-cordes deleted the keep-comments branch August 15, 2022 14:07
@slawekjaranowski slawekjaranowski changed the title Keep comments in .flattened-pom.xml Fix #269 Keep comments in .flattened-pom.xml Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep comments on flattened pom
5 participants