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

[java] ShortVariable should whitelist lambdas #720

Closed
jsotuyod opened this Issue Nov 8, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@jsotuyod
Member

jsotuyod commented Nov 8, 2017

Rule Set:
java-naming

Description:
Lambdas can be short snippets using short variable names for known parameter types (such as calling a String s, or an int i. The rule currently ignores for-loops, but doesn't consider lambdas in the same way.

Code Sample demonstrating the issue:

aListFullOfStrings.forEach(s -> s.replaceAll(" ", ""));

Possible fix (to be thoroughly tested):

[not(ancestor::ForInit)]
**[not(ancestor::LambdaExpression)]** [not(../../VariableDeclarator and ../../../LocalVariableDeclaration and ../../../../ForStatement)]
[not((ancestor::FormalParameter) and (ancestor::TryStatement))]
@nachimehta

This comment has been minimized.

Show comment
Hide comment
@nachimehta

nachimehta Nov 20, 2017

Any official work arounds here? I find that if I create a ruleset.xml file and add

<rule ref="rulesets/java/naming.xml/ShortMethodName">
    <properties>
        <property name="violationSuppressXPath" value=".[ancestor::LambdaExpression]"/>
    </properties>
</rule>

That seems to work, but it also removes a check for shortVariableNames for some reason. i.e.

String s = "helloworld";

Doesn't get caught anymore. I've tried a variety of configurations to no avail.

nachimehta commented Nov 20, 2017

Any official work arounds here? I find that if I create a ruleset.xml file and add

<rule ref="rulesets/java/naming.xml/ShortMethodName">
    <properties>
        <property name="violationSuppressXPath" value=".[ancestor::LambdaExpression]"/>
    </properties>
</rule>

That seems to work, but it also removes a check for shortVariableNames for some reason. i.e.

String s = "helloworld";

Doesn't get caught anymore. I've tried a variety of configurations to no avail.

@nachimehta

This comment has been minimized.

Show comment
Hide comment
@nachimehta

nachimehta Nov 20, 2017

Turns out the error is coming from ShortVariable rule:

<rule ref="rulesets/java/naming.xml/ShortVariable">
    <properties>
        <property
                name="violationSuppressXPath" value=".[ancestor::LambdaExpression]"/>
    </properties>
</rule>

Works beautifully

nachimehta commented Nov 20, 2017

Turns out the error is coming from ShortVariable rule:

<rule ref="rulesets/java/naming.xml/ShortVariable">
    <properties>
        <property
                name="violationSuppressXPath" value=".[ancestor::LambdaExpression]"/>
    </properties>
</rule>

Works beautifully

@jsotuyod jsotuyod added this to the 6.1.0 milestone Nov 30, 2017

@adangel adangel modified the milestones: 6.1.0, 6.2.0 Feb 25, 2018

@oowekyala

This comment has been minimized.

Show comment
Hide comment
@oowekyala

oowekyala Mar 25, 2018

Member

[not(ancestor::LambdaExpression)] ignores all variables declared in a lambda. Maybe we should restrict that to lambda parameters, eg [not(../../LambdaExpression/FormalParameter)] ? If the lambda is a block lambda, then usual naming conventions should probably apply for variable declarations

Member

oowekyala commented Mar 25, 2018

[not(ancestor::LambdaExpression)] ignores all variables declared in a lambda. Maybe we should restrict that to lambda parameters, eg [not(../../LambdaExpression/FormalParameter)] ? If the lambda is a block lambda, then usual naming conventions should probably apply for variable declarations

@adangel adangel modified the milestones: 6.2.0, 6.3.0 Mar 26, 2018

@oowekyala oowekyala referenced this issue Mar 27, 2018

Open

[java] Improve naming conventions rules #972

8 of 14 tasks complete

@jsotuyod jsotuyod changed the title from [java] ShortVariableName should whitelist lambdas to [java] ShortVariable should whitelist lambdas Apr 7, 2018

@adangel adangel modified the milestones: 6.3.0, 6.4.0 Apr 28, 2018

@oowekyala oowekyala self-assigned this May 23, 2018

@oowekyala oowekyala added the has:pr label May 23, 2018

@adangel adangel closed this in #1140 May 23, 2018

adangel added a commit that referenced this issue May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment