Unconditionally disable combining SARIF files for GHES 3.18#2961
Unconditionally disable combining SARIF files for GHES 3.18#2961
Conversation
koesie10
force-pushed
the
koesie10/disable-combine-sarif-files-ghes
branch
from
7d4dc4a to
aafbeb2
Compare
| semver.lt(githubVersion.version, "3.18.0") | ||
| ) { | ||
| return false; | ||
| if (githubVersion.type === GitHubVariant.GHES) { |
There was a problem hiding this comment.
This logic looks good, but it took a while for me to parse it because there there's a double negative: "should disable" and false means it's enabled.
It might make things easier to understand if you negate the function name and all of the return values. So, allowCombinedSarifFiles or something.
It may not actually be any simpler, but maybe it's worth a try.
This is a non-blocking comment.
There was a problem hiding this comment.
I agree that it's somewhat hard to parse, but since being "enabled" is the default case for now, I think having this as a shouldDisable method makes sense for now. Once the feature flag has been removed, I think it would make to make a change to make being "disabled" the default case.
2 participants
This unconditionally disables combining of SARIF files with non-unique categories for GHES 3.18 since this feature is unsupported on GHES 3.18. For dotcom (and GHEC-DR), we'll depend on the feature flag for now.
Merge / deployment checklist