-
Notifications
You must be signed in to change notification settings - Fork 352
Various fixes related to path includes #10762
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?
Various fixes related to path includes #10762
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10762 +/- ##
============================================
+ Coverage 57.38% 57.47% +0.09%
- Complexity 1673 1679 +6
============================================
Files 346 346
Lines 12759 12796 +37
Branches 1209 1212 +3
============================================
+ Hits 7322 7355 +33
- Misses 4972 4976 +4
Partials 465 465
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:
|
790f7ae
to
31679d2
Compare
Signed-off-by: Nicolas Nobelis <[email protected]>
…model Signed-off-by: Nicolas Nobelis <[email protected]>
…udes Signed-off-by: Nicolas Nobelis <[email protected]>
Signed-off-by: Nicolas Nobelis <[email protected]>
31679d2
to
bfb62c7
Compare
val pathIncludes = input.ortResult.getIncludes().findPathIncludes(project, input.ortResult) | ||
val dependencies = input.ortResult.dependencyNavigator.projectDependencies(project) | ||
if (pathExcludes.isEmpty()) { | ||
if (pathExcludes.isEmpty() && pathIncludes.isEmpty()) { |
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 think there is a problem here: pathIncludes.isEmpty()
can mean two things:
- There are no path includes defined at all so this project is included.
- There are path includes but none match this project so it is excluded.
I wonder if for the case that no path includes are defined at all, Includes
should return a default path include which matches **
to make the usage unambiguous.
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 am not too sure about this.
I also experimented with null
to reflect the tri-state, and is the end I went with a separated boolean isExcludedByPathIncludes
in EvaluatedFinding
.
Maybe we can discuss this in the Community meeting.
This PR contains some of the change still required for #10347.
Please have a look at the individual commit messages for the details.
@tsteenbe With these changes, the includes are rendered in the WebApp report: when a result is excluded due to the presence of includes, all includes are rendered in the collapsible section under it, with the mention (includes).