-
Notifications
You must be signed in to change notification settings - Fork 79
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
[EC63] UnnecessarilyAssignValuesToVariables rule on caught exception #230
Conversation
Hi @dedece35 , I checked the list and this PR should be ok regarding the criteria. |
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 👌 Can you update the CHANGELOG?
…- update changelog.
…rilyAssignValuesToVariables_rule_on_caught_exception # Conflicts: # CHANGELOG.md
|
||
public void testCompliantCatchException() throws Exception { |
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.
ok for me, but, please remove "throws Exception" because no Exception really thrown.
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.
@natixis-caen : last modification , then I will approve this PR :p
…bles_rule_on_caught_exception
To discuss in core-team : deprecation of this rule only for Java |
…bles_rule_on_caught_exception
Kudos, SonarCloud Quality Gate passed! |
Why this rule should be deprecated? Already in SonarQube? In fact, there are already 22 Java rules in SonarQube about not using variables/code. https://rules.sonarsource.com/java/tag/unused/RSPEC-1481/ Perhaps EC63 is a kind of duplicate? to be discussed. |
Hi @utarwyn, @glalloue, @jhertout, after checking completyly this rule (currently, only implemented in JAVA), you're right @utarwyn, there is already 2 native SonarQube rules for 2 subjects "unused variable" and "return directly instead of creation of temporary variable" implemented in our ecoCode rule EC63 :
Second thing, all implemented tests show that our EC63 rule is a duplication of 3 previous native Sonarqube rules : in each line of test files that we check if we have an issue reported by our rule, there is also a native Sonarqube rule that reports also an issue. maybe we can use "ecocode" tag to native SaonrQube rules with our tool : https://github.com/green-code-initiative/ecoCode-common/tree/main/tools/rules_config#tagging-scripts for me, we can deprecate this rule, now. OK with me ? |
draft : waiting for #258 |
Hi @natixis-caen and @utarwyn, |
Stop to mark unused exception variables as code smell.