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

Properly prune dumpLicenseInfo reports #57

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Apr 17, 2023

In #55 I used distinct to filter out duplicate entries however it turns out there is a corner case which I missed regarding differing configs, i.e. dependency A in one sbt project its in Test config and in another project its in default config. This could happen because the DepLicense data structure that is derived from updateLicense contains the configs for that given dependency and so if they differ they won't get filtered out by distinct.

This PR solves the problem in a different way, which is to add a dumpLicensePruneDuplicates that works by pruning out the duplicates in the data structure that is used just before actually writing out the contexts. That way we have the behaviour that a user expects, i.e. not having the exact same rows generated in the report.

A test has been added which fails when you set dumpLicensePruneDuplicates to false.

@eed3si9n I have a couple more fixes incoming when I tried to use this within Pekko so I will let you know when to make a new release.

@mdedetrich mdedetrich force-pushed the make-prune-work-multi-config-setup branch 4 times, most recently from 91b0d61 to fb08d51 Compare April 18, 2023 00:01
@@ -106,25 +110,26 @@ object SbtLicenseReport extends AutoPlugin {
)
Seq(config)
},
dumpLicensePruneDuplicates := true,
Copy link
Member

Choose a reason for hiding this comment

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

Default setting should probably go into globalSettings to have the most flexibility - https://www.scala-sbt.org/1.x/docs/Plugins-Best-Practices.html#Provide+default+values+in

Copy link
Member

Choose a reason for hiding this comment

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

I can't imagine a situation where people would want to set this setting to false though. Does this need to be a setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default setting should probably go into globalSettings to have the most flexibility

Done

I can't imagine a situation where people would want to set this setting to false though. Does this need to be a setting?

Only thing I can come up with is debugging, but its not a great answer. I can also remove it if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Yea. Let's remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, also renamed commit to be more precise in its wording.

@mdedetrich mdedetrich force-pushed the make-prune-work-multi-config-setup branch 4 times, most recently from f8b93e9 to fe602a5 Compare April 18, 2023 08:13
@mdedetrich mdedetrich force-pushed the make-prune-work-multi-config-setup branch from fe602a5 to 2cc5c42 Compare April 18, 2023 08:48
@mdedetrich mdedetrich force-pushed the make-prune-work-multi-config-setup branch from 2cc5c42 to aa182e9 Compare April 18, 2023 08:48
@mdedetrich mdedetrich changed the title Add prune setting to filter multiple entries due to different configs Properly prune dumpLicenseInfo reports Apr 18, 2023
Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

2 participants