-
Notifications
You must be signed in to change notification settings - Fork 309
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for PathLicenseMatcher
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
RootLicenseMatcher
to ParentLicenseMatcher
RootLicenseMatcher
to PathLicenseMatcher
Please have a look at the individual commit messages for the details.