-
Notifications
You must be signed in to change notification settings - Fork 353
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
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 <nicolas.nobelis@bosch.com>
...model Signed-off-by: Nicolas Nobelis <nicolas.nobelis@bosch.com>
...udes Signed-off-by: Nicolas Nobelis <nicolas.nobelis@bosch.com>
Signed-off-by: Nicolas Nobelis <nicolas.nobelis@bosch.com>
31679d2 to
bfb62c7
Compare
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.
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.
Am I right that semantics is as follows: A file is included, if it is not matched by any exclude OR if it is matched by at least one include and an arbitrary amount of excludes?
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.
No, excludes always win. A file is included when: (there are no includes defined (which means include all) OR it is matched by an include) AND it is not matched by an exclude.
Uh oh!
There was an error while loading. Please reload this page.
This PR contains some of the change still required for #10347.
Please have a look at the individual commit messages for the details.
image@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).