Skip to content

Conversation

@suvmanda
Copy link
Contributor

@suvmanda suvmanda commented Oct 7, 2020

No description provided.

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

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

<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

</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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants