-
Notifications
You must be signed in to change notification settings - Fork 362
Notice followups #11059
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
base: main
Are you sure you want to change the base?
Notice followups #11059
Conversation
| } | ||
|
|
||
| addApplicableLicenseFiles(licenseFiles) | ||
| addApplicableLicenseFiles(noticeFiles) |
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.
Why should these be treated the same as license files?
(This imho should also have a rationale in the commit msg)
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 the function name is just misleading. We use the same function for adding any files, including patentFiles and otherLicenseFiles. So it seems natural to also use it for noticeFiles, and I don't think a justification is needed.
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, the function docs say "Return a mapping from the given relative [directories] to the relative paths of the (root) licenses file". So, the result is considered a "root license" file. An I suppose (haven't checked), that the returned stuff is treated like a root license file. For example when inserting original license texts into a notice. E.g. would this change here alter
- what the main license of a package is (also in SPDX report
- what is put into the notice 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.
Or does this change only lead to the files being include by the file archiver, but does not have any effect otherwise?
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.
Or does this change only lead to the files being include by the file archiver, but does not have any effect otherwise?
That's at least exactly the intention of this change. Also see this Slack conversation.
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.
This is called by matchWithRootLicenses() and looking at the callers of that one, to me it is not clear why there should be no effect on those. Can you please explain?
This is a fixup for a71ed8b. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
cedf1b7 to
00dac6f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11059 +/- ##
=========================================
Coverage 57.42% 57.43%
Complexity 1701 1701
=========================================
Files 346 346
Lines 12835 12838 +3
Branches 1215 1215
=========================================
+ Hits 7370 7373 +3
Misses 4998 4998
Partials 467 467
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:
|
Relates to #6399. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
00dac6f to
462130a
Compare
Please have a look at the individual commit messages for the details.