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

Wrong newlines in functional style #346

Merged
merged 14 commits into from
Oct 7, 2020

Conversation

aktsay6
Copy link
Collaborator

@aktsay6 aktsay6 commented Sep 30, 2020

What's done:

  • Fixed bugs

This pull request closes #321 #326

Actions checklist

  • Implemented Rule, added Warnings
  • Added tests on checks
  • Added tests on fixers
  • Updated diktat-analysis.yml
  • Updated available-rules.md

### What's done:
  * Fixed bugs
@aktsay6 aktsay6 added the bug Something isn't working label Sep 30, 2020
@aktsay6 aktsay6 requested review from petertrr and kentr0w September 30, 2020 12:19
@aktsay6 aktsay6 marked this pull request as draft September 30, 2020 12:38
@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #346 into master will decrease coverage by 0.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #346      +/-   ##
============================================
- Coverage     82.92%   82.90%   -0.02%     
- Complexity     1224     1239      +15     
============================================
  Files            60       60              
  Lines          3027     3047      +20     
  Branches        968      973       +5     
============================================
+ Hits           2510     2526      +16     
  Misses          170      170              
- Partials        347      351       +4     
Flag Coverage Δ Complexity Δ
#unittests 82.90% <81.81%> (-0.02%) 1239.00 <10.00> (+15.00) ⬇️

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

Impacted Files Coverage Δ Complexity Δ
...rg/cqfn/diktat/ruleset/rules/files/NewlinesRule.kt 86.80% <81.81%> (-1.10%) 90.00 <10.00> (+15.00) ⬇️

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 bbbe011...ccbecac. Read the comment docs.

Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

Take a look at comments. Also, there is another issue in this very logic: #326 . Will it be automatically fixed by change from this PR?

### What's done:
  * Fixed bugs
@aktsay6 aktsay6 marked this pull request as ready for review October 2, 2020 08:49
@aktsay6 aktsay6 requested a review from petertrr October 2, 2020 08:49
@aktsay6 aktsay6 linked an issue Oct 2, 2020 that may be closed by this pull request
### What's done:
  * Fixed bugs
@aktsay6 aktsay6 requested a review from petertrr October 2, 2020 14:21
@@ -330,6 +332,10 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
private fun ASTNode.getParentExpressions() =
parents().takeWhile { it.elementType in chainExpressionTypes && it.elementType != LAMBDA_ARGUMENT }

private fun isMultilineLambda(node: ASTNode): Boolean =
node.findAllNodesWithSpecificType(LAMBDA_ARGUMENT).firstOrNull()?.text?.count { it == '\n' } ?: -1 > 0
Copy link
Member

Choose a reason for hiding this comment

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

Won't findAllNodesWithSpecificType trigger on some nested lambdas or lambdas further in a call chain?

### What's done:
  * Fixed bugs
  * Call chains logic remade
@aktsay6 aktsay6 requested a review from petertrr October 5, 2020 10:16
@aktsay6 aktsay6 requested a review from petertrr October 7, 2020 09:42
}
}
?.dropWhile { !it.treeParent.textContains('(') && !it.treeParent.textContains('{') }
// fixme: we can't distinguish fully qualified names from chain of calls for now
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// fixme: we can't distinguish fully qualified names from chain of calls for now
// fixme: we can't distinguish fully qualified names (like java.lang) from chain of property accesses (like list.size) for now

Just to be sure we don't forget what this was about


@Test
@Tag(WarningNames.WRONG_NEWLINES)
fun `should trigger on non-multiline lambdas`() {
Copy link
Member

Choose a reason for hiding this comment

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

Please add example like

list.filter {

}
.map(::foo).filter {
    bar()
}

It should be considered incorrect, when multiline lambda is not the first call.

### What's done:
  * Fixed bugs
Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

LGTM

@aktsay6 aktsay6 merged commit d185e2b into master Oct 7, 2020
@aktsay6 aktsay6 deleted the bugfix/wrong-newlines-functional-style(#321) branch October 7, 2020 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants