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

Multiline string indent #1052

Merged
merged 10 commits into from
Oct 24, 2021
Merged

Conversation

paul-dingemans
Copy link
Collaborator

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:

  • 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.

Paul Dingemans added 2 commits January 9, 2021 17:30
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
Paul Dingemans added 2 commits July 31, 2021 14:35
# Conflicts:
#	ktlint-ruleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt
@paul-dingemans
Copy link
Collaborator Author

@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.

@shashachu
Copy link
Contributor

@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?

@paul-dingemans
Copy link
Collaborator Author

Functionally there is no difference between raw string literals below:

val string1 = 
    """
    line1
    line2
    """.trimIndent()

val string2 = 
    """
        line1
        line2
    """.trimIndent()

val string3 = """
        line1
        line2
    """.trimIndent()

val string4 = """
        line1
        line2
""".trimIndent()

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?

@shashachu
Copy link
Contributor

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 ktlint tests difficult to write when we wanted to test indentation linting or formatting (as you saw we need to disable the rule on our own tests). I don't think that's something that we need to make configurable.

But, I do think that it is ok to align the opening and closing quotes while keeping contents untouched.

@paul-dingemans
Copy link
Collaborator Author

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.
A simple setting like kotlin_indent_multiline_string_template (boolean) would allow to choose the format of your liking and enforcing consistent formatting in the project.

@paul-dingemans
Copy link
Collaborator Author

From Kotlin documentation (https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.text/trim-indent.html):

Detects a common minimal indent of all the input lines, removes it from every line and also removes the first and the last lines if they are blank (notice difference blank vs empty).

Note that blank lines do not affect the detected indent level.

In case if there are non-blank lines with no leading whitespace characters (no indent at all) then the common indent is 0, and therefore this function doesn't change the indentation.

Doesn't preserve the original line endings.

This PR basically does the same thing with the big difference that the code looks clean and consistent.

@cldfzn
Copy link

cldfzn commented Aug 21, 2021

@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?

@paul-dingemans
Copy link
Collaborator Author

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.

@romtsn
Copy link
Collaborator

romtsn commented Aug 24, 2021

@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 trimIndent function, but we are focused on the source code rather. I've also seen people aligning the multiline strings like this, which would be changed by this PR

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.

@paul-dingemans
Copy link
Collaborator Author

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.

I will do so after PR #1230 is accepted. The functionality of aligning start and end quotes become part of the indent rule.

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.

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.

maybe we should still think about introducing another ruleset for situations like this, but make it opt-in.

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.

@paul-dingemans
Copy link
Collaborator Author

paul-dingemans commented Sep 19, 2021

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.

@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?

@paul-dingemans
Copy link
Collaborator Author

@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:

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.

@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
Copy link
Collaborator

romtsn commented Oct 15, 2021

hey @paul-dingemans, I appreciate your patience really 😄 sorry again.
Do you mean the tabs are being replaced by spaces in the string templates?

@paul-dingemans
Copy link
Collaborator Author

paul-dingemans commented Oct 15, 2021

Do you mean the tabs are being replaced by spaces in the string templates?

Yes, tabs in the indentation margin of the string template are replaced with spaces tot resolve issue 575.

@romtsn
Copy link
Collaborator

romtsn commented Oct 15, 2021

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.

Paul Dingemans added 3 commits October 16, 2021 18:05
# Conflicts:
#	ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRule.kt
…ral except for the line with the closing quotes
@paul-dingemans
Copy link
Collaborator Author

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.

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.
@paul-dingemans
Copy link
Collaborator Author

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?

@romtsn
Copy link
Collaborator

romtsn commented Oct 24, 2021

Yeah, we can hold off before the new release (cc @shashachu)

@romtsn romtsn merged commit 27f90a4 into pinterest:master Oct 24, 2021
@paul-dingemans paul-dingemans deleted the multiline-string-indent branch October 30, 2021 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants