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

profile added #13

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

profile added #13

wants to merge 4 commits into from

Conversation

suvmanda
Copy link
Contributor

@suvmanda suvmanda commented Oct 7, 2020

No description provided.

lmarniazman
lmarniazman suggested changes Oct 7, 2020
View reviewed changes
Copy link
Member

@lmarniazman lmarniazman left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! Just a few comments regarding empty lines, if you could adjust those it would be perfect and I can approve.

pom.xml Outdated
</execution>
</executions>
</plugin>

Copy link
Member

Choose a reason for hiding this comment

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

Could you format this so that there is only one empty line between different plugins? Just for consistency

pom.xml Outdated
</plugins>
</build>
</profile>

Copy link
Member

Choose a reason for hiding this comment

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

The other profiles don't have an empty line between them, we should keep it the same way here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmarniazman
Empty lines removed.Thnaks

pom.xml
</reporting>

<profiles>
<!-- JaCoCo for test coverage -->
Copy link
Member

Choose a reason for hiding this comment

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

@hohwille Was this how you meant to include the jacoco-report profile into maven-parent? I thought this was a good way of doing it. After this PR is merged, we can also delete the profile from the pom.xml of sonar-devon4j-plugin.

lmarniazman
lmarniazman suggested changes Oct 7, 2020
View reviewed changes
pom.xml
@@ -343,9 +343,6 @@
<warName>${project.artifactId}</warName>
</configuration>
</plugin>

Copy link
Member

Choose a reason for hiding this comment

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

I meant that tags on the second level should have one line between them, tags on every other level should not. So if you coud add one empty line between 345-346 that would be great

pom.xml
@@ -423,7 +420,6 @@
</plugin>
</plugins>
</reporting>

Copy link
Member

Choose a reason for hiding this comment

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

See review comment above, one empty line here between </reporting> and <profiles> would be perfect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lmarniazman . All comments are fixed.

@hohwille
Copy link
Member

hohwille commented Nov 13, 2020 •
edited
Loading

You could really help me by writing a description what is changed by the PR and why in 1-3 sentences.
So looking at the diffs, you moved jacoco coverage to a profile.
Why has this been done? Is there a related issue?
The profile is not active anymore by default so more or less you disabled the code-coverage.
We have many projects derived from this parent pom.
Therefore we need to be very careful about such changes as they might break things elsewhere.

Was this PR done because of devonfw/devon4j#300?
So devon4j is already derived from this maven-parent and therefore already has javacoco active. Why do we need this PR then? Are there any implicit requirements for sonarcloud that I am not aware? Or this this PR just a missunderstanding?

@suvmanda
Copy link
Contributor Author

suvmanda commented Dec 2, 2020

You could really help me by writing a description what is changed by the PR and why in 1-3 sentences.
So looking at the diffs, you moved jacoco coverage to a profile.
Why has this been done? Is there a related issue?
The profile is not active anymore by default so more or less you disabled the code-coverage.
We have many projects derived from this parent pom.
Therefore we need to be very careful about such changes as they might break things elsewhere.

Was this PR done because of devonfw/devon4j#300?-Yes
So devon4j is already derived from this maven-parent and therefore already has javacoco active. Why do we need this PR then? Are there any implicit requirements for sonarcloud that I am not aware? Or this this PR just a missunderstanding?

As @lmarniazman suggestion,Was this how you meant to include the jacoco-report profile into maven-parent. we thought this was a good way of doing it. After this PR is merged, we can also delete the profile from the pom.xml of sonar-devon4j-plugin.

hohwille added a commit that referenced this pull request May 31, 2021
@hohwille
#13: fixed indendation
603d596
@hohwille
Copy link
Member

I incorporated the indendation fixes into master.
For the profile it has to be active by default as otherwise we will break derived projects.
I still think this PR is a missunderstanding. However, if we make the profile active by default this PR can be merged. Otherwise simply close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmarniazman lmarniazman lmarniazman 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.
3 participants
@suvmanda @hohwille @lmarniazman