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

Rename RootLicenseMatcher to ParentLicenseMatcher #9302

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

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 sschuberth requested a review from a team as a code owner October 18, 2024 07:53
@sschuberth sschuberth enabled auto-merge (rebase) October 18, 2024 07:53
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.28%. Comparing base (03b4ed9) to head (75a737c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../main/kotlin/commands/GetPackageLicensesCommand.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9302      +/-   ##
============================================
- Coverage     67.77%   67.28%   -0.50%     
- Complexity     1187     1201      +14     
============================================
  Files           239      239              
  Lines          7821     8446     +625     
  Branches        904      905       +1     
============================================
+ Hits           5301     5683     +382     
- Misses         2153     2394     +241     
- Partials        367      369       +2     
Flag Coverage Δ
funTest-docker 60.07% <ø> (+0.96%) ⬆️
funTest-non-docker 33.63% <0.00%> (-1.08%) ⬇️
test 37.36% <88.88%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sschuberth
Copy link
Member Author

@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(
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

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.

@fviernau
Copy link
Member

@fviernau mind having a look as I have some follow-up changes?

Sorry, missed that my comment was in "pending" state.

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