Extract and refactor call formatter - #677
Conversation
| import org.jetbrains.kotlin.psi.psiUtil.startsWithComment | ||
| import org.jetbrains.ktfmt.format.ParseError | ||
|
|
||
| interface CallFormatter : KotlinAstFormatter { |
There was a problem hiding this comment.
Note for the future: please consider spelling explicitly in the commit whether the refactoring (e.g. extraction) is purely mechanical (same code, some names, same comments) or implies some additional non-trivial (e.g. not "moved it from visitX to formatX method") changes.
It will simplify the review process for housekeeping things a lot. Thanks!
| import com.google.googlejavaformat.Indent | ||
| import com.google.googlejavaformat.Output | ||
|
|
||
| sealed class Indentation { |
There was a problem hiding this comment.
Please add a KDoc: an example of what it is and what it represents (I understand it represents an indentation, but what exactly? In some units, as a forced break, or as something to operate on to properly make a GJF decision for block calls? Now to answer that I look into the usages)
| import org.jetbrains.ktfmt.format.visitor.Indentation.Companion.ZERO | ||
| import org.jetbrains.ktfmt.format.visitor.Indentation.Companion.makeCond | ||
|
|
||
| interface CallFormatter : KotlinAstFormatter { |
There was a problem hiding this comment.
Also, a KDoc per our convention, please: what part of grammar it owns, what it formats, etc.
| val blockComments = | ||
| bodyExpression.children().filter { it is PsiComment && it.text.startsWith("/*") }.toList() | ||
|
|
||
| val hasBody = expressionStatements.isNotEmpty() || blockComments.isNotEmpty() |
There was a problem hiding this comment.
// note to self: blockComments only. :120 hasBody considers all comments
|
passing it to @kunyavskiy. Cannot do it manually because of GH orgs shenanigans |
b91f652 to
a7b0c49
Compare
Purely mechanical change that extracts all `visit*` methods related to call expressions into corresponding `format*`
…f `formatFunctionCall`
a7b0c49 to
dbdc3a4
Compare
| class Const(val value: Int) : Indentation() { | ||
| override val indent: Indent = Indent.Const.make(value, 1) | ||
|
|
||
| operator fun plus(other: Const): Const = Const(value + other.value) |
There was a problem hiding this comment.
Probably this can be done on demand but this functions can be also in super-class, just by adding/subtracting/multiplying both indents in conditional.
It's also a bit wierd API, that plus/minus accepts other Const, while times accepts int.
But, I'm fine with adding all of this on-demand, espically, if we wouldn't consider this class as public API (should it be internal than?)
There was a problem hiding this comment.
Most of these functions were added on demand, yes
should it be internal than?
It depends in general on whether we consider formatters implementations a public API
| * 2. '... = Runnable @Annotation { ... }' due to the annotation | ||
| */ | ||
| val KtExpression?.scopingLambda: KtLambdaExpression? | ||
| internal data class ScopingLambda( |
There was a problem hiding this comment.
Why is this change in commit about indentations?
There was a problem hiding this comment.
Missed during commit reorg
| builder.block(expressionBreakIndent) { | ||
| // allows adjusting arguments indentation if a break will be made | ||
| val nameTag = BreakTag() | ||
| for ((ktExpression, openingGroups, closingGroups, isTrailingLambda, isLast) in |
There was a problem hiding this comment.
Formatting is wierd here
| * | ||
| * Each group is then emitted one by one to the [builder] while opening and closing groups. Each | ||
| * group is opened **before** a corresponding expression is emitted and closed **after**. However, | ||
| * if an expression represents a function call, e.g. `doIt(1, 2) { it }`, the group is closed |
There was a problem hiding this comment.
It would be nice to explain why.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
kunyavskiy
left a comment
kunyavskiy
left a comment
There was a problem hiding this comment.
Actually, I'd say non of my comments above is very important. Feel free to adress them separately in upcoming MRs, if you see it's more convenient for your workflow.
3 participants
Indentationwrappers, mostly to simplify some code. GJF's indents have everything private, so we can't provide customoperator funs for them, requiring us to constantly keep a lot of indent instancesGroupInfointo a separate fileCallFormatter's existing implementationsNote: this PR is purely refactoring, none of the behaviours are changed. I'm planning to extract changes from #634 into a separate
KotlikLangCallFormatterin the next PR