-
Notifications
You must be signed in to change notification settings - Fork 50
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
Migrate all SARIF readers to extend from SarifReader #70
Conversation
@darkspirit510 - This changes look really good to me, and much cleaner. I'd like to compare the scores this change generates against the scores generated before this merge before I merge it just to make sure that no scores inadvertently change because of all this refactoring. I hope to do that tonight. |
@darkspirit510 Precaution definitely does some Java analysis. I put up a PR in BenchmarkJava on how to use. Maybe this will help. |
@davewichers any news on this PR? |
Sorry, I was on holiday for a week and then lost track of this. I just tested and didn't see any score changes so merged. Thanks for doing this! |
This PR migrates all SARIF reader classes extending them from SarifReader class. I moved basically all the reader's code to the parent class and used constructor instead of abstract methods. That's why this PR deletes lots of code 🙈 (Special thanks to @ericwb who "kind of" made me think that way).
PS: I am super sorry for all the contributers of SARIF classes for deleting most of their code, but I think it's way cleaner now.
PPS: @ericwb Does Precaution support Java, yet? I was able to verify changes because I do not have a valid result file for Precaution and running it on my own has an empty result set 🤷♂️