-
Notifications
You must be signed in to change notification settings - Fork 313
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
Avoid parsing SARIF file when workaround for duplicate notification locations is disabled #1697
Conversation
DISABLE_DUPLICATE_LOCATION_FIX - this is to avoid needless crashes on large sarif files
@henrymercer — I think it is backwards compatible, but I'm not sure I'm right. Do you have an opinion? |
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.
Nice, thanks for the fix! For posterity, I suggest we rename this PR to something like "Avoid parsing SARIF file when workaround for duplicate notification locations is disabled".
Agreed 👍 |
#1696 fixes the broken PR checks. Once that's in, we should be able to merge this. |
@henrymercer From my viewpoint, this could be merged now — it seems like you merged in the fix to get tests to pass. Let's merge it? |
I agree this is good to go. I'll merge this now since you're happy with it, but in the future feel free to merge or auto-merge yourself once you're happy and after an approving review. |
DISABLE_DUPLICATE_LOCATION_FIX - this is to avoid needless crashes on
large sarif files
Merge / deployment checklist