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

[MPIR-375] add plugin excludes feature for plugin-management report #7

Closed
wants to merge 3 commits into from

Conversation

belingueres
Copy link
Contributor

Added pluginManagementExcludes list parameter.

Added pluginManagementExcludes list parameter.
@rfscholte
Copy link
Contributor

Fixed in 82f4bf2
Thanks for the PR!

@rfscholte rfscholte closed this Jan 11, 2019
<!--
Example plugin generating the Eclipse's "Plugin execution not covered by lifecycle configuration" error message
-->
<plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this plugin, I don't see the need for it in this IT

}
else
{
MavenProject pluginProject =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you the try-catch statement inside the else-statement, not around the whole if-else statement


for ( String pattern : excludes )
{
String[] subStrings = pattern.split( ":" );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a reason to cleanup the pattern. This is already a list, not a single comma-separated String.

* @return <code>true</code> if the artifact matches the pattern
*/
protected boolean compareDependency( String pattern, Artifact artifact )
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Use org.apache.maven.shared.artifact.filter.PatternExcludesArtifactFilter instead

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 didn't knew that class...very handy!

<version>1.0-SNAPSHOT</version>
<packaging>jar</packaging>
<name>plugin management project info</name>
<dependencies>
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to remove all unrequired stuff, like dependencies

- Removed unnecessary code in tests.
- Narrowed scope of try-catch when building the report.
- Simplified exclusion logic by using PatternExcludesArtifactFilter.
@belingueres
Copy link
Contributor Author

Pushed a new commit containing the suggested changes.

@rfscholte rfscholte reopened this Jan 15, 2019
@rfscholte
Copy link
Contributor

I've reopened this issue, so I can merge the improvements on the code. Can you first solve the conflicts?

@michael-o
Copy link
Member

michael-o commented Aug 20, 2019

Merged with 82f4bf2.

@michael-o michael-o closed this Aug 20, 2019
asfgit pushed a commit that referenced this pull request Dec 15, 2019
…lte:

- Removed unnecessary code in tests.
- Narrowed scope of try-catch when building the report.
- Simplified exclusion logic by using PatternExcludesArtifactFilter.
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