Skip to content
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

Issue #463: extended ReturnCountExtendedCheck to accept lambdas #513

Merged
merged 1 commit into from
Dec 5, 2016

Conversation

rnveach
Copy link
Contributor

@rnveach rnveach commented Dec 3, 2016

Issue #463

Lambdas are now supported.
Also 100% code coverage. Regression showed no NPEs.

<module name="ReturnCountExtendedCheck">
<property name="maxReturnCount" value="1"/>
<property name="ignoreMethodLinesCount" value="0"/>
<property name="minIgnoreReturnDepth" value="99"/>
<property name="ignoreEmptyReturns" value="false"/>
<property name="topLinesToIgnoreCount" value="0"/>
</module>

One thing to note, lambdas don't have a method name, so the violation message currently prints that as null. Should we drop that part of the message? Should we drop support for matching these patterns?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 98.337% when pulling 2cae467 on rnveach:issue_463 into 8525f2a on sevntu-checkstyle:master.

@romani
Copy link
Member

romani commented Dec 3, 2016

Should we drop that part of the message?
Return count for 'null' lambda is 3 (max allowed is 1).

yes, only for lambda.
Message should be clear to user.

Should we drop support for matching these patterns?

I do not understand question, please rephrase the question.

@rnveach
Copy link
Contributor Author

rnveach commented Dec 3, 2016

@romani We have property ignoreMethodsNames, which drops methods from checking if they match a name.
Lambdas have no name, so can they still use this property to ignore lambdas?
Right now I convert null to "null" so you can put in the string null to the property to drop these.

@romani
Copy link
Member

romani commented Dec 3, 2016

ok, lets keep that hack for now, real code will show us right way to treat it.

@rnveach
Copy link
Contributor Author

rnveach commented Dec 3, 2016

@romani If we are keeping null method names for ignoring, does it make sense to still keep null in the message display. Users should understand that it is the method name there since it is similar to other messages from this Check. If they see null then they may understand that they can ignore it by add "null" to the ignore method list.

@romani
Copy link
Member

romani commented Dec 3, 2016

does it make sense to still keep null in the message display.

No sense. We could mention this in javadoc as side effect and not a contract for behavior and could be changed in future

@rnveach rnveach force-pushed the issue_463 branch 2 times, most recently from 0917f66 to 496a960 Compare December 4, 2016 00:49
@rnveach
Copy link
Contributor Author

rnveach commented Dec 4, 2016

@romani Done.
Had to split violations to logViolation because of NestedIfDepth violation.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 98.338% when pulling 496a960 on rnveach:issue_463 into c03e3d4 on sevntu-checkstyle:master.

@romani
Copy link
Member

romani commented Dec 4, 2016

please rebase.

@rnveach
Copy link
Contributor Author

rnveach commented Dec 4, 2016

@romani Done.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 98.338% when pulling 75df30c on rnveach:issue_463 into b3f59ad on sevntu-checkstyle:master.

@romani
Copy link
Member

romani commented Dec 4, 2016

update to functionality is big, please provide diff report.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 98.338% when pulling 768976a on rnveach:issue_463 into b3f59ad on sevntu-checkstyle:master.

@rnveach
Copy link
Contributor Author

rnveach commented Dec 5, 2016

Update is because regression showed the message was incorrect. I removed the method name but didn't decrease the parameter number in the property file.
We probably should create a test for something like this.

@romani Here is regression.
http://rveach.no-ip.org/checkstyle/regression/reports/128/

@romani romani merged commit 4e33ce1 into sevntu-checkstyle:master Dec 5, 2016
@rnveach rnveach deleted the issue_463 branch December 5, 2016 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants