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

[MRELEASE-1107] Respect line separator also on release:prepare #154

Closed
wants to merge 1 commit into from
Closed

[MRELEASE-1107] Respect line separator also on release:prepare #154

wants to merge 1 commit into from

Conversation

Captain-P-Goldfish
Copy link
Contributor

The line separator config should also be used on mvn release:prepare and not just on mvn release:prepare-with-pom

@michael-o
Copy link
Member

Can you please create a JIRA issue for this? Can you also describe when you experienced this?

@Captain-P-Goldfish Captain-P-Goldfish changed the title Respect line separator also on release:prepare [MRELEASE-1107] Respect line separator also on release:prepare Sep 30, 2022
@Captain-P-Goldfish
Copy link
Contributor Author

Issue is opened: https://issues.apache.org/jira/browse/MRELEASE-1107

... and I experienced it when using release:prepare. The configured linefeeds were not respected. And after looking at the source code, the problem was clear.

michael-o
michael-o requested changes Sep 30, 2022
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Do you see a reasonable way to test this also like MRELEASE-899?

maven-release-plugin/src/main/java/org/apache/maven/plugins/release/PrepareReleaseMojo.java Outdated Show resolved Hide resolved
maven-release-plugin/src/main/java/org/apache/maven/plugins/release/PrepareReleaseMojo.java Outdated Show resolved Hide resolved
@Captain-P-Goldfish
Copy link
Contributor Author

Alright I fixed the code. I simply tried to repair the unit tests that were broken with my changes instead of fixing it within the maincode which was indeed bad style. Sorry for that.

@Captain-P-Goldfish
Copy link
Contributor Author

Do you see a reasonable way to test this also like MRELEASE-899?

There are actually already tests for this purpose. But only if you insist I can check if additional tests should be added.

@michael-o
Copy link
Member

Do you see a reasonable way to test this also like MRELEASE-899?

There are actually already tests for this purpose. But only if you insist I can check if additional tests should be added.

Can you point me which those are? I am on a short leash this week.

@Captain-P-Goldfish
Copy link
Contributor Author

org.apache.maven.plugins.release.PrepareReleaseMojoTest#testLineSeparatorInPrepareWithPom

@Captain-P-Goldfish
Copy link
Contributor Author

I saw the test was for prepare-with-poms only. I added an additional test for release:prepare

@michael-o michael-o self-requested a review October 15, 2022 18:54
@asfgit asfgit closed this in b1840a4 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 Awaiting requested review from michael-o

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Captain-P-Goldfish @michael-o