-
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
Bugfix. Bug in autofix in string templates #780
Conversation
### What's done: * Fixed bug
Codecov Report
@@ Coverage Diff @@
## master #780 +/- ##
============================================
- Coverage 80.93% 80.92% -0.01%
- Complexity 2194 2203 +9
============================================
Files 98 98
Lines 5653 5666 +13
Branches 1747 1753 +6
============================================
+ Hits 4575 4585 +10
- Misses 284 285 +1
- Partials 794 796 +2
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 bug
templateEntries.forEach { node -> | ||
if (!node.text.contains("\n") && node.text.isNotBlank()) { | ||
val correctedText = StringBuilder() | ||
when { |
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.
very overloaded logic!
need to split it somehow (methods?)
or at least (!!) move node.treePrev.elementType in stringLiteralTokens
to variable/method
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.
Moved it to method
@@ -18,3 +18,10 @@ val text = | |||
x | |||
""" | |||
|
|||
val dockerFileAsText = |
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.
not enough tests, you have created more conditions than in this testcase
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.
Added to fix tests
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.
simplify logic
### What's done: * Fixed bug
|
||
val dockerFileAsText = | ||
""" | ||
FROM $baseImage |
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.
@aktsay6 the issue was about a bug with disappearing spaces. We shouldn't change indentation of different lines of a string, because this way we obviously change the string itself. We could only move all strings at once, if there is trimIdent
or trimMargin
.
### What's done: * Fixed bug
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.
@aktsay6 please fix code style and we'll merge it
### What's done: * Fixed code style
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 change the logic
correctedText.append(node.firstChildNode.text.trimEnd()) | ||
} | ||
// if string template is after literal_string | ||
node.treePrev.elementType !in stringLiteralTokens && node.treeNext.elementType in stringLiteralTokens -> { |
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 create two expressions for node.treePrev.elementType in stringLiteralTokens
and node.treeNext.elementType in stringLiteralTokens
. And unify this logic
### What's done: * Fixed code style
### What's done: * Fixed code style
node.treePrev.elementType in stringLiteralTokens && node.treeNext.elementType !in stringLiteralTokens -> { | ||
correctedText.append(node.firstChildNode.text.trimEnd()) | ||
} | ||
isPrevStringTemplate && !isNextStringTemplate -> correctedText.append(node.firstChildNode.text.trimEnd()) |
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.
Can these conditions be true simultaneously?
@@ -290,16 +283,10 @@ class IndentationRule(configRules: List<RulesConfig>) : DiktatRule( | |||
private fun ASTNode.getExceptionalIndentInitiator() = treeParent.let { parent -> | |||
when (parent.psi) { | |||
// fixme: custom logic for determining exceptional indent initiator, should be moved elsewhere | |||
is KtDotQualifiedExpression -> { | |||
// get the topmost expression to keep extended indent for the whole chain of dot call expressions |
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.
This comment shouldn't be extracted outside of the code block. Is diktat doing this now? Seems like a bug
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.
May be a possible bug. I will open an issue
### What's done: * Fixed code style
### What's done: * Fixed code style
### What's done: * Fixed code style
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.
lgtm
What's done:
This pull request closes #766