-
Notifications
You must be signed in to change notification settings - Fork 39
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
Correct fix with string concat #946
Conversation
### What's done: Fixed bug Added comment and tests
wrongStringTemplate.node.treeParent.replaceChild(wrongStringTemplate.node, KotlinParser().createNode("$firstPart\" +\n\"$secondPart")) | ||
val correctNode = | ||
try { | ||
KotlinParser().createNode("$firstPart\" +\n\"$secondPart") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to just not put any pluses in ${}
block, no?
Need to check if we are in ${}
write now or not. If yes - add a new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just afraid that simple parsing of code can be inaccurate, but not sure, may be other reviewers will be ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to suggest the same. We can find the current node by offset, and if it is text - add concatenation, else - simply add a line break
Codecov Report
@@ Coverage Diff @@
## master #946 +/- ##
============================================
+ Coverage 83.85% 83.93% +0.08%
- Complexity 2380 2392 +12
============================================
Files 101 101
Lines 6025 6050 +25
Branches 1777 1785 +8
============================================
+ Hits 5052 5078 +26
Misses 264 264
+ Partials 709 708 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
### What's done: Fixed logic to resolve bug
### What's done: Fix to build
### What's done: Fix to build
### What's done: Updated
diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
Outdated
Show resolved
Hide resolved
@@ -457,7 +480,7 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule( | |||
val hasNewLineBefore: Boolean, | |||
val indexLastSpace: Int = 0) : LongLineFixableCases() | |||
|
|||
class StringTemplate(val node: ASTNode, val delimiterIndex: Int) : LongLineFixableCases() | |||
class StringTemplate(val node: ASTNode, val delimiterIndex: Int, val multiLineOffset: Int) : LongLineFixableCases() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, make multiLineOffset
Int?
to better indicate, that string is single line if it is null
instead of 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be better when we check if multiLineOffset == 0
, but when we calculate this variable, it will be worse
diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
Outdated
Show resolved
Hide resolved
### What's done: Fixed after review
@@ -293,6 +294,7 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule( | |||
val incorrectText = wrongStringTemplate.node.text | |||
val firstPart = incorrectText.substring(0, wrongStringTemplate.delimiterIndex) | |||
val secondPart = incorrectText.substring(wrongStringTemplate.delimiterIndex, incorrectText.length) | |||
// wrongStringTemplate.multiLineOffset equals zero if string in one line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what I meant :) Why do you check if split is at white space only for single-line string?
### What's done: Fixed after review
private fun fixStringTemplate(wrongStringTemplate: LongLineFixableCases.StringTemplate) { | ||
val incorrectText = wrongStringTemplate.node.text | ||
val firstPart = incorrectText.substring(0, wrongStringTemplate.delimiterIndex) | ||
val secondPart = incorrectText.substring(wrongStringTemplate.delimiterIndex, incorrectText.length) | ||
wrongStringTemplate.node.treeParent.replaceChild(wrongStringTemplate.node, KotlinParser().createNode("$firstPart\" +\n\"$secondPart")) | ||
// wrongStringTemplate.multiLineOffset equals zero if string in one line | ||
// if wrongStringTemplate.multiLineOffset isn't equal to 0, it means that string is multiline, which means that split should be without quote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With or without? If we are inside a multiline string, we should split it with quote and concatenation, shouldn't we? Also, for multiline strings we are not checking any methods like trimIndent()
:
val string = """
first line
veryy looooooooooooooooong second line
""".trimIndent()
how will we split it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the remarks and merge
### What's done: Changed logic
### What's done: Fixed to pass all tests
### What's done: Fixed according to our code style
### What's done: Fixed according to our code style
### What's done: Fixed according to our code style
diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
Outdated
Show resolved
Hide resolved
diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
Show resolved
Hide resolved
diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
Show resolved
Hide resolved
### What's done: Fixed after review
What's done: