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.

Sign up for GitHub

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

Jump to bottom

Always preserve dominant node value (even if empty) #217

Merged
merged 1 commit into from Oct 15, 2022
Merged

Always preserve dominant node value (even if empty) #217

merged 1 commit into from Oct 15, 2022

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Sep 12, 2022

This closes #216

@kwin
Copy link
Contributor Author

kwin commented Sep 12, 2022

I tested this against the Maven Integration Tests and there were no failures.

@michael-o
Copy link
Member

Can you run this change against all Maven Plugin Tools ITs?

michael-o
michael-o approved these changes Oct 10, 2022
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This make sense. Requirement for merge is that MPT are running fine. Waiting for confirmation then I can merge.

@michael-o michael-o marked this pull request as ready for review October 10, 2022 12:54
@kwin
Copy link
Contributor Author

kwin commented Oct 10, 2022

Can you run this change against all Maven Plugin Tools ITs?

You mean https://github.com/apache/maven-plugin-tools being build with Prun-its after referencing this PR?

@michael-o
Copy link
Member

Can you run this change against all Maven Plugin Tools ITs?

You mean https://github.com/apache/maven-plugin-tools being build with Prun-its after referencing this PR?

Correct, this code must be in use for these ITs.

@michael-o
Copy link
Member

@kwin Please rebase against master.

@kwin
Copy link
Contributor Author

kwin commented Oct 10, 2022

@michael-o Actually this was thought as an alternative to #213 (as this includes blank values), but we can also obviously apply it on top.

@michael-o
Copy link
Member

@michael-o Actually this was thought as an alternative to #213 (as this includes blank values), but we can also obviously apply it on top.

I see it as a compliment for #213 therefore a rebase is required.

@kwin kwin force-pushed the bugfix/preserve-dominant-node-value-even-if-empty branch 2 times, most recently from f112155 to 4f5e2f2 Compare October 11, 2022 06:15
@sonatype-lift
Copy link

sonatype-lift bot commented Oct 11, 2022

⚠️ 22 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@kwin
Copy link
Contributor Author

kwin commented Oct 11, 2022

Build is successful with Prun-itsof https://github.com/apache/maven-plugin-tools after upgrading to this version of plexus-utils.
TBH I don't think there is an IT for this config merging behaviour in MPT.

michael-o pushed a commit to kwin/plexus-utils that referenced this pull request Oct 15, 2022
@kwin @michael-o
Xpp3DomUtils#mergeIntoXpp3Dom() must not override the dominant value …
1d660b5 
…in case it is empty (codehaus-plexus#216)

This codehaus-plexus#216 and fixes codehaus-plexus#217
@michael-o michael-o force-pushed the bugfix/preserve-dominant-node-value-even-if-empty branch from 4f5e2f2 to 1d660b5 Compare October 15, 2022 18:38
@michael-o michael-o force-pushed the bugfix/preserve-dominant-node-value-even-if-empty branch from 1d660b5 to 67ac243 Compare October 15, 2022 18:39
@michael-o michael-o merged commit 67ac243 into codehaus-plexus:master Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Xpp3DomUtils#mergeIntoXpp3Dom() must not override the dominant value in case it is empty
4 participants
@kwin @michael-o @gnodet @guylabs