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

Doesn't report Tab character(s) in multiline string #575

Closed
schalkms opened this issue Sep 7, 2019 · 6 comments
Closed

Doesn't report Tab character(s) in multiline string #575

schalkms opened this issue Sep 7, 2019 · 6 comments

Comments

@schalkms
Copy link

schalkms commented Sep 7, 2019

Please take a look at the following multiline string example.
The tabs in the multiline string don't get reported by ktlint 0.34.2.

package ktlint.sample

public const val str = """
	Tab at the beginning of this line
Tab	 in the middle of this string
Tab at the end of this line.	
"""
3flex pushed a commit to detekt/detekt that referenced this issue Sep 29, 2019
In the past this repo used tabs instead of 4 spaces for .kt files.
Ktlint doesn't convert tabs in multiline strings to spaces.
Please take a look at the following issue.
pinterest/ktlint#575

Therefore, the following command has been used for conversion.

find ./ -iname '*.kt' -type f -exec bash -c 'expand -t 4 "$0" | sponge "$0"' {} \;

* ./ is recursively searching the given directory
* -iname is a case insensitive match .kt files
* type -f finds only regular files (no directories, binaries or symlinks)
* -exec bash -c executes the following commands in a subshell for each file
* expand -t 4 expands all tabs to 4 spaces
* sponge soaks up standard input (from expand) and writes it to the same file

Source:
https://stackoverflow.com/a/43523362
sowmyav24 pushed a commit to sowmyav24/detekt that referenced this issue Oct 1, 2019
In the past this repo used tabs instead of 4 spaces for .kt files.
Ktlint doesn't convert tabs in multiline strings to spaces.
Please take a look at the following issue.
pinterest/ktlint#575

Therefore, the following command has been used for conversion.

find ./ -iname '*.kt' -type f -exec bash -c 'expand -t 4 "$0" | sponge "$0"' {} \;

* ./ is recursively searching the given directory
* -iname is a case insensitive match .kt files
* type -f finds only regular files (no directories, binaries or symlinks)
* -exec bash -c executes the following commands in a subshell for each file
* expand -t 4 expands all tabs to 4 spaces
* sponge soaks up standard input (from expand) and writes it to the same file

Source:
https://stackoverflow.com/a/43523362
smyachenkov pushed a commit to smyachenkov/detekt that referenced this issue Dec 9, 2019
In the past this repo used tabs instead of 4 spaces for .kt files.
Ktlint doesn't convert tabs in multiline strings to spaces.
Please take a look at the following issue.
pinterest/ktlint#575

Therefore, the following command has been used for conversion.

find ./ -iname '*.kt' -type f -exec bash -c 'expand -t 4 "$0" | sponge "$0"' {} \;

* ./ is recursively searching the given directory
* -iname is a case insensitive match .kt files
* type -f finds only regular files (no directories, binaries or symlinks)
* -exec bash -c executes the following commands in a subshell for each file
* expand -t 4 expands all tabs to 4 spaces
* sponge soaks up standard input (from expand) and writes it to the same file

Source:
https://stackoverflow.com/a/43523362
smyachenkov pushed a commit to smyachenkov/detekt that referenced this issue Dec 9, 2019
In the past this repo used tabs instead of 4 spaces for .kt files.
Ktlint doesn't convert tabs in multiline strings to spaces.
Please take a look at the following issue.
pinterest/ktlint#575

Therefore, the following command has been used for conversion.

find ./ -iname '*.kt' -type f -exec bash -c 'expand -t 4 "$0" | sponge "$0"' {} \;

* ./ is recursively searching the given directory
* -iname is a case insensitive match .kt files
* type -f finds only regular files (no directories, binaries or symlinks)
* -exec bash -c executes the following commands in a subshell for each file
* expand -t 4 expands all tabs to 4 spaces
* sponge soaks up standard input (from expand) and writes it to the same file

Source:
https://stackoverflow.com/a/43523362
@paul-dingemans
Copy link
Collaborator

I don't think that the initial example given should report an error:

public const val str = """
	Tab at the beginning of this line
Tab	 in the middle of this string
Tab at the end of this line.	
"""

This multiline string is not suffixed with .trimIndent() nor .trimMargin(). So the entire content of the line should be preserved.
Note: the indentation rule of ktlint currently ignores the .trimMargin() suffix entirely.

Also no error should be reported in case the tab character is found on the right hand side of the margin like in example below:

    @Test
    fun `format multiline string with tabs after the margin is indented properly`() {
        val code =
            """
            val str = ${'"'}${'"'}${'"'}
                 ${'\t'}Tab at the beginning of this line but after the right hand side of the indentation margin
                 Tab${'\t'}in the middle of this string
                 Tab at the end of this line.${'\t'}
                 ${'"'}${'"'}${'"'}.trimIndent()
                 """.trimIndent()
        assertThat(IndentationRule().lint(code)).isEmpty()
    }

Only in case the tab character is found on the left hand side of the margin the error should be reported:

    @Test
    fun `format multiline string with tabs before the margin is indented properly`() {
        val code =
            """
            val str = ${'"'}${'"'}${'"'} 
${'\t'}       Tab at the beginning of this line but on the left hand side of the indentation margin
                 Tab${'\t'}in the middle of this string
                 Tab at the end of this line.${'\t'}
                 ${'"'}${'"'}${'"'}.trimIndent()
                 """.trimIndent()
        assertThat(IndentationRule().lint(code)).isEqualTo(
            listOf(
                LintError(2, 11, "indent", "Unexpected tab character(s) in indent of multiline string"),
            )
        )
    }

Can you agree with above? I have not yet prepared the PR because the calculation of the position of the unexpected tab needs to be determined. Before I do that, I would like to have confirmation about this approach.

@schalkms
Copy link
Author

Thanks for the clarification. I understand the reasons behind this.
For me it is fine if the maintainers (from Pinterest) declare this as system behavior.
Since this issue exists for quite some time, who can help with the decision here to get this resolved? - @romtsn

Note: the indentation rule of ktlint currently ignores the .trimMargin() suffix entirely.

I didn't know that. Thanks for the hint.

@romtsn
Copy link
Collaborator

romtsn commented Dec 27, 2020

Yeah, I agree that we shouldn't touch the contents of raw strings if they are not using trimIndent as it might be that they are using tabs intentionally (and it's hard for us to distinguish between intentional/unintentional usage).

But it would be beneficial to detect those in the beginning of the line and where trimIndent is present, so @paul-dingemans your test is describing it well (with a caveat that this shouldn't be a violation in case indent_style=tab)

@paul-dingemans
Copy link
Collaborator

Ok, I am already working on a fix. It will be ready early 2021.

paul-dingemans pushed a commit to paul-dingemans/ktlint that referenced this issue Jan 9, 2021
* Tabs after indentation margin are not reported as unexpected
* If all lines of the multiline string are indented with tabs
  only then replace the tabs with spaces
* Report multiline strings for which the indentations contain
  both tabs and spaces as this can not autocorrected with
  certainty
* Indent multiline strings when used as property value

'format-raw-string-trim-indent.kt.spec'
* Replicate some tests cases as separate unit test for ease
  of testing
* Fixed error as code was not valid kotlin
* Add indentation errors
paul-dingemans pushed a commit to paul-dingemans/ktlint that referenced this issue Jan 9, 2021
@paul-dingemans
Copy link
Collaborator

As discussed in above PR, the indent rule will never change the indentation inside a raw string literal. Although it can be very useful to fix indentation inside string literals (appended with trimIndent), it is expected that such behaviour is not well expected in the entire community. Therefore this behaviour will not be added to the indent rule which is contained inside the standard ruleset.
@romtsn Will you close this issue, as it was your decision not to resolve it?

I am working on a publicly available custom ruleset in which such a rule will be provided. Either this rule will be provided via a separate repository or it might be included in the experimental ruleset of ktlint. For more info see issue #1229 and PR #1230.

@romtsn
Copy link
Collaborator

romtsn commented Oct 30, 2021

Yeah, @schalkms sorry, but we aren't going to support indentation within string templates for now

@romtsn romtsn closed this as completed Oct 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants