-
Notifications
You must be signed in to change notification settings - Fork 26
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
refactor: LPVSScanossDetectService refactoring and unit test fix #524
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
============================================
+ Coverage 92.28% 92.33% +0.05%
- Complexity 524 529 +5
============================================
Files 48 48
Lines 1736 1736
Branches 210 209 -1
============================================
+ Hits 1602 1603 +1
Misses 69 69
+ Partials 65 64 -1 ☔ View full report in Codecov by Sentry. |
b170f74
to
45dff0a
Compare
45dff0a
to
b148997
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.
Please check my comments. Thanks!
@@ -261,6 +181,96 @@ public List<LPVSFile> checkLicenses(LPVSQueue webhookConfig) { | |||
return detectedFiles; | |||
} | |||
|
|||
private Set<LPVSLicense> buildLicenseSet(ScanossJsonStructure object) { |
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.
Please add Javadoc comments for all functions (including private).
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.
Javadoc comments generated by GenAI are added. Please let me know if I have anything to revise!
+ ".json")); | ||
} | ||
Reader reader = getReader(webhookConfig); | ||
|
||
// convert JSON file to map | ||
Map<String, ArrayList<Object>> map = |
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.
It's possible to avoid usage of gson object here:
Map<String, ArrayList<Object>> map = new Gson().fromJson(reader, new TypeToken<Map<String, ArrayList<Object>>>() {}.getType());
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.
Done. Thanks for your precise review!
} | ||
|
||
private ScanossJsonStructure getScanossJsonStructure(Gson gson, String content, LPVSFile file) { | ||
ScanossJsonStructure object = gson.fromJson(content, ScanossJsonStructure.class); |
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.
It's possible to replace input parameter Gson gson
and avoid usage of gson object like this:
ScanossJsonStructure object = new Gson().fromJson(content, ScanossJsonStructure.class);
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.
Done. Thanks for your precise review!
Signed-off-by: Taewan Kim <t25.kim@samsung.com>
a162cc5
to
3df2eab
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.
Thank you for the code refactoring!
Pull Request
Description
The
checkLicenses
has been refactored due to long method.The unit tests,
testWithNullHeadCommitSHA
andtestWithNullHeadCommitSHADB
, have been fixed to read the existing test file.Type of change
Testing
This PR was verified by the unit test.
Test Configuration:
Checklist: