-
Notifications
You must be signed in to change notification settings - Fork 239
SQ Plugin: Remove old runtime version checks #9560
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
SQ Plugin: Remove old runtime version checks #9560
Conversation
|
|
mary-georgiou-sonarsource
left a comment
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 left a comment about a test that seems to still use version < 9.9.
Please also recheck again because I see there are more occurrences of Version.create( for versions < 9.9. For Example in CSharpPluginTest.getExtensions
| assertThat(getSecurityStandards(Version.create(9, 4), PCI_DSS_RULE_KEY)).isEmpty(); | ||
| } | ||
|
|
||
| @Test |
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.
What about this test?
Seems to be testing version 9.5.
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.
The version number passed here is not used in our Java codebase, it's just passed to the Plugin API. So there's not much we can do about it. I removed the test for version 9.4, because the minimum API version is 9.9, but this test should still work. I could change the version number to 9.9, but it wouldn't matter; at least it shows from which version this feature is available.
The same is true for other occurrences I didn't remove: the runtime versions are passed to the API, and you can only see them in the code from the decompiled .class files. We cannot remove them.
mary-georgiou-sonarsource
left a comment
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



Fixes #7798