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

Bugfix: proper newlines for return type #1122

Merged
merged 14 commits into from
Nov 16, 2021

Conversation

kgevorkyan
Copy link
Member

@kgevorkyan kgevorkyan commented Nov 12, 2021

What's done:

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

### What's done:
* Provide test
### What's done:
* WIP
### What's done:
* Codestyle
### What's done:
* Fixes
@codecov
Copy link

codecov bot commented Nov 12, 2021

Codecov Report

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

Impacted file tree graph

@@             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.

@kgevorkyan kgevorkyan linked an issue Nov 15, 2021 that may be closed by this pull request
### What's done:
* Fix logic
### What's done:
* Code style
### What's done:
* Fixes
@kgevorkyan kgevorkyan marked this pull request as ready for review November 15, 2021 11:51
it.elementType == COMMA &&
!it.treeNext.run { elementType == WHITE_SPACE && textContains('\n') }
(it.elementType == COMMA && !it.treeNext.isNewLineNode()) ||
// Move RPAR to the new line, if there is exist return type == TYPE_REFERENCE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (commaOrRpar.elementType == COMMA) {
nextWhiteSpace?.treeNext?.let {
commaOrRpar.appendNewlineMergingWhiteSpace(nextWhiteSpace, nextWhiteSpace.treeNext)
} ?: commaOrRpar.treeNext?.treeParent?.appendNewlineMergingWhiteSpace(nextWhiteSpace, commaOrRpar.treeNext)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

### What's done:
* Review
### What's done:
* Fix
### What's done:
* Fixes
### What's done:
* Fixes
### What's done:
* Fixes
@kgevorkyan kgevorkyan merged commit 5e2e2ed into master Nov 16, 2021
@kgevorkyan kgevorkyan deleted the bugfix/newline_for_return_type branch November 16, 2021 08:52
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.

Proper newlines for return type
2 participants