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

Added option to ignore a set of group IDs from the report #168

Merged
merged 4 commits into from
Jun 8, 2022
Merged

Added option to ignore a set of group IDs from the report #168

merged 4 commits into from
Jun 8, 2022

Conversation

mudkiplex
Copy link
Contributor

Fixes #116

@davvelar
Added option to ignore a set of group IDs from the report
de8d809 
Change-Id: I098a8bc3d177798bb3237dff918bec493116934a
jaredsburrows
jaredsburrows reviewed Sep 29, 2021
View reviewed changes
src/main/kotlin/com/jaredsburrows/license/LicenseReportTask.kt Outdated
@@ -40,6 +40,7 @@ internal open class LicenseReportTask : DefaultTask() { // tasks can't be final
@Input var copyCsvReportToAssets = false
@Input var copyHtmlReportToAssets = false
@Input var copyJsonReportToAssets = false
@Input var ignoredGroupIds: Set<String> = setOf()
Copy link
Owner

Choose a reason for hiding this comment

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

@Input var ignoredGroupIds = setOf<String>() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely

jaredsburrows
jaredsburrows reviewed Sep 29, 2021
View reviewed changes
src/main/kotlin/com/jaredsburrows/license/LicenseReportTask.kt Outdated
@@ -167,6 +168,12 @@ internal open class LicenseReportTask : DefaultTask() { // tasks can't be final
val pomFile = resolvedArtifact.file
val node = xmlParser.parse(pomFile)

// If group ID is part of ignored groups, skip this artifact
val groupId = node.getAt("groupId").text().trim()
Copy link
Owner

Choose a reason for hiding this comment

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

Add test for this?

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'm not particularly versed in Groovy that so that'll take me some time to figure out. I'll get back to you.

@davvelar
Fixed comments
4778e8a 
* Added unit tests for ignoring one and all group IDs respectively.
mudkiplex
mudkiplex commented Sep 30, 2021
View reviewed changes
src/test/groovy/com/jaredsburrows/license/LicensePluginAndroidSpec.groovy Outdated
@@ -1212,4 +1212,171 @@ final class LicensePluginAndroidSpec extends Specification {
taskName << ['licenseDebugReport', 'licenseReleaseReport']
copyEnabled << [true, false]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaredsburrows Do you want me to add these tests to the LicensePluginJavaSpec too?

@mudkiplex
Copy link
Contributor Author

Hello @jaredsburrows, It's been a couple of months, and I'm wondering if you plan to merge my pull request. If not, I'll just leave it be, otherwise I'll squash the commits, rebase and put a new one on top of latest master for you to review.

@jaredsburrows
Copy link
Owner

@mudkiplex Can you please sync this?

@davvelar
Merge remote-tracking branch 'origin/master' into ignore_group_ids
f019c83 
Fixed conflicts:
* Used new file structure.
* Fixed unit tests.
@mudkiplex
Copy link
Contributor Author

@jaredsburrows I believed I've managed to sync with latest master correctly. Please have a look.

@jaredsburrows jaredsburrows merged commit afde395 into jaredsburrows:master Jun 8, 2022
@jaredsburrows
Copy link
Owner

Thanks!

@mudkiplex Should we consider adding ignoredNames for the dependencies specifically?

@jaredsburrows
Copy link
Owner

Also we should probably update the README.md

@mudkiplex mudkiplex deleted the ignore_group_ids branch June 9, 2022 06:50
@mudkiplex
Copy link
Contributor Author

Thanks!

@mudkiplex Should we consider adding ignoredNames for the dependencies specifically?

We could do that. I'm not sure what format the ignoredNames should have - name + group as a concatenated string? A map between groups and names?

@jaredsburrows
Copy link
Owner

@mudkiplex It would be the same thing except filtering by artifactId instead of groupId.

@mudkiplex
Copy link
Contributor Author

mudkiplex commented Jun 10, 2022 •
edited
Loading

@jaredsburrows What I mean is that dependencies come as "group:name:version", and there might be several groups that have artifacts with the same name. So we could have:

  1. group1:name1:version
  2. group2:name1:version

If we were to just filter on name here we might accidentally filter out some dependencies that we want to keep.

An alternative could be to work with exclusion patterns instead. That way we could let the user decide to filter on groups, names or entire artifacts. So instead of e.g this (The effect here would be that all artifacts are removed):

dependencies {
  implementation 'some.group:some.artifact:1.2.3'
  implementation 'some.group:some.other.artifact.1.2.3'
  implementation 'some.other.group:some.artifact:1.2.3'
}
licenseReport {
  ignoredGroupIds = ["some.group"] // Removes entire group
  ignoredArtifactIds = ["some.artifact"] // Removes all artifacts with that name
}

We could have either one of:

licenseReport {
  ignoredPatterns = ["some.group:some.artifact"] // Removes only that particular artifact
  ignoredPatterns = ["some.group"] // Removes entire group
  ignoredPatterns = ["some.artifact"] // Removes all artifacts with that name
}

Does that make sense?

@mudkiplex
Copy link
Contributor Author

Created a new PR for this here: #218

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

@jaredsburrows jaredsburrows jaredsburrows left review comments

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

Successfully merging this pull request may close these issues.

Would like a way to exclude certain group ids from the report
3 participants
@mudkiplex @jaredsburrows @davvelar