-
Notifications
You must be signed in to change notification settings - Fork 512
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
Multiline string indent #1052
Multiline string indent #1052
Conversation
The indenting margin is determined analog to the trimIndent function. Spaces and tabs are both handled as single indent characters. The indentation of the closing quotes is not relevant unless the opening and closing quotes are on different lines but does not contain any content which is weird but still valid Kotlin code. The 'IndentationRuleTrimIndentTest' is added to clarify and validate the behavior of the trimIndent function. Content of a multiline string should not be indented with respect to the closing quotes (note that the opening quotes can be on a previous line, for example after an assignment). File 'format-raw-string-trim-indent.kt.spec' is changed for following reasons: * Example code was not valid Kotlin code * Changed a comment in an example to explain why the autocorrected code actually looks worse than the original. This will be fixed in a next PR.
…dentTest.kt.txt Avoid breaking the build as long as the ktlint-disable directive on the package statement is not recognized. See pull request pinterest#1038
# Conflicts: # ktlint-ruleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt
@shashachu This PR is already waiting a long time for review and merge. I hope that you have time to look at this PR. I have 4 others PR which are dependent on the merge of this specific PR. A lot of indentation problems will be solved when those PR's get merged. Once the PR's are merged, I am willing to investigate and resolve all other indentation issues as well. |
@paul-dingemans: @romtsn and I chatted a bit about this PR, and in general we feel that we shouldn't be modifying the indentation inside of string templates, which is what your PR is doing. However, fixing the indentation of the opening and closing quotes seems fine because that doesn't modify the contents of the string. Would you like to try just making that change? |
Functionally there is no difference between raw string literals below:
Personally I do like the format of string1 notation the most. But I do understand that opinions may vary. Is it an option to support different styles using a editorconfig setting so that projects can choose their preferred style? |
Although they're functionally equivalent, I do think it's a good rule to follow to never change the indentation of the contents of a string template. e.g. it even makes our own But, I do think that it is ok to align the opening and closing quotes while keeping contents untouched. |
I can not recall that the rules had to be disabled on the ktlint tests except for the one test that proves that indents of a string template can be removed. I have reformatted all unit tests that did violate this changed rule regarding string templates and did not encounter any difficulties with it. The big advantage of this change is that it makes multiline string templates consistent in the entire project. |
From Kotlin documentation (https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.text/trim-indent.html):
This PR basically does the same thing with the big difference that the code looks clean and consistent. |
@paul-dingemans I see you have a few branches on your fork, considering the length of time these pull requests have been open do you think it would be a better option to create a separate rule-set to publish at this point? |
I prefer this to be fixed in the standard rule. I can only hope that enough people will support this PR and that it will be merged. I have several other PR's available that are solving other indent problems but they depend on this PR to be merged first. @shashachu and @romtsn is there any chance that this PR gets merged or should I just close it as you do not support it? My last suggestion was to make it configurable to fix indentation inside a multiline string template. |
@paul-dingemans been meaning to write here for months, sorry for that. Your work is really appreciated, but please try to open an issue/discussion beforehand to better align with the project goals, so you don't waste your time implementing something that will eventually be rejected. In general, I like the PR, and I would gladly use the feature myself (aligning the contents of a multiline string), but the kotlin guidelines and IJ itself are very vague here (understandably), so I'm afraid we can face some tension when introducing this feature to the standard ruleset. What you referred to in #1052 (comment) is actually a runtime behavior of the val str =
"""
test
test
test
""".trimIndent() We'd like to keep configuration at a minimum, hence introducing a new config option and further bloating the .editorconfig for something opinionated like this PR isn't what we want. That being said, If you'd be up for changing your PR in a way, that it only aligns the opening/closing quotes but keeps the content unchanged, that'd be great - if not, I can pick it up and finish accordingly. @shashachu @Tapchicoma maybe we should still think about introducing another ruleset for situations like this, but make it opt-in. |
I will do so after PR #1230 is accepted. The functionality of aligning start and end quotes become part of the indent rule.
I am glad you like it. The reason that I want to have this functionality is that my work projects have lots of multiline raw string literals which are indented in different way. Mostly this is caused by refactoring code. Whenever a multiline raw string literal is moved to some other place in the code (especially when moved to another indentation level), the inner indentation of the string gets more out of sync.
As described in #1229 I have created a custom rule which only changes the inner indentation of the multiline raw string literal. This rule can be added to the experimental rule set or another rule set so everyone can opt in. Otherwise I will just create a separate (public) rule set repository for this. |
@romtsn When preparing this change, I found that as part of issue 575, the indentation margin is already manipulated a bit by replacing tab characters in the indentation margin with spaces. What are you thoughts about this. Should it be kept for backwards compatibility or should this also be removed from the standard ruleset? |
@romtsn I had liked to close down this issue before the PR is celebrating its first birthday ;-). So I would be glad, if you can spent a little time to answer my question above which is repeated here as well for your convenience:
|
hey @paul-dingemans, I appreciate your patience really 😄 sorry again. |
Yes, tabs in the indentation margin of the string template are replaced with spaces tot resolve issue 575. |
I think we can remove that, and close the issue, I think we still want to stick to not touching the contents of a string template. |
# Conflicts: # ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRule.kt
…ral except for the line with the closing quotes
…e incorrectly indented
Okay, I have modified the PR. The indentation margin of a multiline raw string literal is not changed anymore with exception of the line that contains the closing quotes. |
...ndard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/ParameterListWrappingRuleTest.kt
Show resolved
Hide resolved
...d/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTrimIndentTest.kt.txt
Outdated
Show resolved
Hide resolved
...uleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt
Show resolved
Hide resolved
...uleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt
Show resolved
Hide resolved
The indentation margin is no longer been altered by the indent rule. So there is no need anymore to verify the behavior of the trimIndent function.
I have three more PR's which are dependent on this change to be merged to master. May I ask to merge this PR and give me a few days to submit those PR's before creating a new Ktlint release as requested in #1254? |
Yeah, we can hold off before the new release (cc @shashachu) |
The indenting margin is determined analog to the trimIndent function. Spaces and tabs are both handled as single indent characters. The indentation of the closing quotes is not relevant unless the opening and closing quotes are on different lines but does not contain any content which is weird but still valid Kotlin code. The 'IndentationRuleTrimIndentTest' is added to clarify and validate the behavior of the trimIndent function.
Note: build currently breaks until PR ktlint-disable is merged.
Content of a multiline string should not be indented with respect to the closing quotes (note that the opening quotes can be on a previous line, for example after an assignment).
File 'format-raw-string-trim-indent.kt.spec' is changed for following reasons: