fix(freemarker): Remove uncategorized licenses from category filter #10231
Conversation
Before, `filterForCategory()` returned all licenses that were uncategorized. While this behavior is documented, the behavior is not expected for a function with this name, and it seems like this was an unintentional change in [1]. [1]: ab43f69 Signed-off-by: Marcel Bochtler <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10231 +/- ##
=========================================
Coverage 56.57% 56.57%
Complexity 1610 1610
=========================================
Files 330 330
Lines 12388 12388
Branches 1172 1172
=========================================
Hits 7008 7008
Misses 4910 4910
Partials 470 470
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I will fix the failing test after a reviewer agrees with me that this is the way this function should handle uncategorized licenses. |
| */ | ||
| @Suppress("UNUSED") // This function is used in the templates. | ||
| fun filterForCategory(licenses: Collection<ResolvedLicense>, category: String): List<ResolvedLicense> = | ||
| licenses.filter { resolvedLicense -> | ||
| input.licenseClassifications[resolvedLicense.license]?.contains(category) != false | ||
| input.licenseClassifications[resolvedLicense.license]?.contains(category) ?: false |
There was a problem hiding this comment.
The notice templates still uses filterForCategory(), and IIUC these intends to include exactly the licenses which are either / or:
- uncategorized
- categorized with the (provided)
include-in-noticecategory.
I suppose that this behavior shall stay as is. So, I think this PR should account for that.
There was a problem hiding this comment.
Not sure if I understand you correctly.
Is your proposal
- Create a new function that returns the uncategorized licenses in addition to the proposed change? And additionally adjust the notice template?
Or is it - Closing this PR and keeping the functionality as-is?
There was a problem hiding this comment.
Not sure if I understand you correctly.
ORT has the files NOTICE_DEFAULT.ftl and NOTICE_SUMMARY.ftl.
I believe that the output should of these templates should remain unaffected by this PR.
This could be achieved with option #1 above IIUC.
|
Question: |
I believe it should be marked breaking. But still there is a risk that it goes unnoticed. |
Maybe add a mandatory |
Before,
filterForCategory()returned all licenses that were uncategorized. While this behavior is documented, the behavior is not expected for a function with this name, and it seems like this was an unintentional change in 1.