Bugfix: proper newlines for return type

[kgevorkyan]

What's done:

• Fix logic
• Add tests
• Update code over the project according latest changes

[codecov]

Codecov Report

Merging #1122 (aa2719a) into master (8914d8b) will increase coverage by 0.09%.
The diff coverage is 95.10%.

@@             Coverage Diff              @@
##             master    #1122      +/-   ##
============================================
+ Coverage     83.43%   83.53%   +0.09%     
- Complexity     2465     2472       +7     
============================================
  Files           102      102              
  Lines          6153     6170      +17     
  Branches       1842     1844       +2     
============================================
+ Hits           5134     5154      +20     
+ Misses          274      270       -4     
- Partials        745      746       +1     

Flag	Coverage Δ
unittests	83.53% <95.10%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...n/kotlin/org/cqfn/diktat/common/cli/CliArgument.kt	0.00% <0.00%> (ø)
...lin/org/cqfn/diktat/plugin/maven/DiktatBaseMojo.kt	70.17% <ø> (ø)
...cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt	96.49% <ø> (ø)
.../ruleset/rules/chapter3/StringConcatenationRule.kt	73.62% <ø> (ø)
...tlin/org/cqfn/diktat/ruleset/utils/AstNodeUtils.kt	84.67% <ø> (ø)
...iktat/ruleset/rules/chapter3/files/NewlinesRule.kt	78.57% <84.61%> (+0.71%)	⬆️
...diktat/ruleset/rules/chapter2/kdoc/KdocComments.kt	86.09% <89.47%> (+2.06%)	⬆️
...g/cqfn/diktat/ruleset/rules/chapter1/FileNaming.kt	72.00% <100.00%> (ø)
.../diktat/ruleset/rules/chapter1/IdentifierNaming.kt	82.02% <100.00%> (ø)
...qfn/diktat/ruleset/rules/chapter1/PackageNaming.kt	87.50% <100.00%> (ø)
... and 65 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8914d8b...aa2719a. Read the comment docs.

[petertrr]

I think, return type is not required here. IF function returns Unit, then it still should follow this style:

fun foo(arg1: Foo,
            arg2: Foo
) {
    // body
}

[kgevorkyan]

I thought about this, and the first version were used such logic: append new line anytime before RPAR

But while I changed the code according this rule over the project, I met, that probably such cases seems appropriate without new line.

Since in this case, we don't need to point out to the reader to something important by adding this extra line, because there are only brackets on it

For example let take less test-based example, than yours, the following seems to me more prettier

private fun deleteSpaces(node: ASTNode,
                         rightSide: Boolean) {

Than this:

private fun deleteSpaces(node: ASTNode,
                         rightSide: Boolean
) {

But, if you are still disagree with me, I can revert logic and add new lines anytime

[petertrr]

Ok, but what about this code:

private fun deleteSpaces(
    node: ASTNode,
    rightSide: Boolean) {
    // method body starts here
}

vs this:

private fun deleteSpaces(
    node: ASTNode,
    rightSide: Boolean
) {
    // method body starts here
}

I think in the latter case parameter list and method body are separated more clearly.

[kgevorkyan]

Ok, don't check for return type for now, add new line after RPAR always

[petertrr]

This looks a little tricky (accessing treeParent, but only if treeNext != null?). Can you provide some comments here?

[kgevorkyan]

Basically, the previous logic is holds: we append new line before nextWhiteSpace if it exists, otherwise before current commaOrRpar node.
Previously were the following:

comma.appendNewlineMergingWhiteSpace(nextWhiteSpace, nextWhiteSpace?.treeNext ?: comma.treeNext)

but it was incorrect and lead to the NPE, since when we would like to append node before comma.treeNext
we should call this method on the parent node, and not on comma node itself