-
Notifications
You must be signed in to change notification settings - Fork 314
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
feat(model): Add the property Issue.affectedPath
#7963
Conversation
This property can be set in case the issue is limited to a file or directory. This prepares for making use of it for ScanCode timeout issues, e.g. filter out irrelevant timeout issues if outside the VCS path or matched by a path exclude. Signed-off-by: Frank Viernau <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7963 +/- ##
=========================================
Coverage 67.01% 67.01%
Complexity 2041 2041
=========================================
Files 357 357
Lines 17103 17103
Branches 2443 2443
=========================================
Hits 11462 11462
Misses 4623 4623
Partials 1018 1018
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM, but I'd like @mnonnenmacher to also approve.
val severity: Severity = Severity.ERROR, | ||
|
||
/** | ||
* The affected file or directory the issue is limited to, if any. |
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.
Maybe "the issue is related to" or "that causes the issue"?
* The affected file or directory the issue is limited to, if any. | ||
*/ | ||
@JsonInclude(JsonInclude.Include.NON_NULL) | ||
val affectedPath: String? = null |
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'm not so happy with the name. affectedPath
sounds like the file at this location is affected by the issue. For example, if a file system is corrupt, then files can be affected by this. But with the example of a ScanCode timeout, the timeout does not affect the file at all, the file is merely the cause of the issue because ScanCode cannot handle it for some reason.
Since we do not know what kinds of issues might make use of this property in future, shall we just call it filePath
or path
?
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.
But with the example of a ScanCode timeout, the timeout does not affect the file at all
Well, I'm reading it differently, like "What thing was affected by the timeout? The file, because it's contents were not properly scanned."
Since we do not know what kinds of issues might make use of this property in future, shall we just call it
filePath
orpath
?
As mentioned on Slack I was against filePath
as also directories could be affected, and path
sounds overly generic, though I could live with it. Maybe relatedPath
?
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 remove myself / my preference from this discussion and will implement what you guys agree @mnonnenmacher @sschuberth.
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.
If both of you are happy with the current name please just go ahead.
This property can be set in case the issue is limited to a file or directory. This prepares for making use of it for ScanCode timeout issues, e.g. filter out irrelevant timeout issues if outside the VCS path or matched by a path exclude.
Part of: #7921.