Rename RootLicenseMatcher to PathLicenseMatcher
#9302
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9302 +/- ##
=========================================
Coverage 67.49% 67.49%
Complexity 1198 1198
=========================================
Files 241 241
Lines 8495 8495
Branches 900 900
=========================================
Hits 5734 5734
Misses 2399 2399
Partials 362 362
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@fviernau mind having a look as I have some follow-up changes? |
| @@ -94,7 +94,7 @@ internal class GetPackageLicensesCommand : OrtHelperCommand( | |||
|
|
|||
| val detectedLicense = curatedFindings.toSpdxExpression() | |||
|
|
|||
| val rootLicense = RootLicenseMatcher().getApplicableRootLicenseFindingsForDirectories( | |||
| val rootLicense = ParentLicenseMatcher().getApplicableLicenseFindingsForDirectories( | |||
There was a problem hiding this comment.
Hm, parent only refers to the direct ancestor.
Just throwing in the idea to use MainLicenseMatcher.
But not sure if this is good, what do you think?
There was a problem hiding this comment.
I believe "Main" has the same notion as "Root" in that it can be interpreted to only mean the license in the top-level directory. More ideas:
- AncestorLicenseMatcher
- InheritedLicenseMatcher
- ApplicableLicenseMatcher
- PathLicenseMatcher
Out of these, probably PathLicenseMatcher is the simplest and least repetitive WRT the class method names.
Any preferences from our side, @fviernau?
There was a problem hiding this comment.
Puh, I'm having a hard time deciding this (except that I would not use Applicable because also licenses in other files can be applicable). Let's maybe do a quick poll on the preferred terminology. @mnonnenmacher @tsteenbe any preferences / ideas?
There was a problem hiding this comment.
At least @mnonnenmacher is on vacation, and I'd like to proceed with this before he's back in two weeks.
There was a problem hiding this comment.
I went for PathLicenseMatcher now.
There was a problem hiding this comment.
to capture that the license is somewhat originating from special files
I believe that "License" in "PathLicenseMatcher" already makes clear enough that only special files, namely license files (which are located along the path to the root), are taken into account.
The term "main license" to me is more something that makes sense in the scope of a whole package, where you want to emphasize that some licenses are more important than others in that they refer to the whole package vs. only a few files in the package.
There was a problem hiding this comment.
The term "main license" to me is more something that makes sense in the scope of a whole package, where you want to emphasize that some licenses are more important than others in that they refer to the whole package vs. only a few files in the package.
"The whole package", isn't that the only use case this matcher is about?
There was a problem hiding this comment.
From reading the test cases I thought that the use-case would be a more generic one: To check the licenses for any path, and the root directory (which implies the package license) only being a special case. That's also why I proposed the renaming in the first place.
So for example, if you have no licenses files in the root, but a dirA/LICENSE file, and you ask for the license of dirA/dirB, then it would be the one of dirA/LICENSE. Or am I misunderstanding something?
There was a problem hiding this comment.
The algorithm normally starts at the VCS path (node) of the package. Looks inside that node for license files.
If found returns, if not, go up. Repeat until the root.
The idea is: Normally the special files from the repository root apply. But in some cases, additionally such special files are placed in subdirs. And we treat this as override. Note the best, but a rather common example is a docs dir containing some Creative commons license file.
There was a problem hiding this comment.
And the use case we have implemented: Detected copyright findings which do not have a nearby detected license they can be associated with, get associated with the "main" licenses of the package. And this PathLicenseMatcher is determining these main licenses, starting at the VCS path of the package.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Sebastian Schuberth <[email protected]>
The name `RootLicenseMatcher` was confusing as not necessarily license files from the root are matched: If no license files are in the root, but a license file is in a parent directory on the path to the root directory, that license would be applicable. Reflect that in several renamings that avoid the "root" term if not appropriate. Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth
changed the title
RootLicenseMatcher to ParentLicenseMatcherRootLicenseMatcher to PathLicenseMatcher
Please have a look at the individual commit messages for the details.