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

Indentation rule fixes #1051

Conversation

paul-dingemans
Copy link
Collaborator

This PR fixes the indentation problems mentioned in #575 and tab indentation problems. Also a lot of unit tests have been added to check the tab indentation.

Examples of code blocks will be auto corrected by merging this PR.

Do not report tabs after indentation margin in multiline string (#575):

            val str =
                """
                ${'\t'}Tab at the beginning of this line but after the indentation margin
                Tab${'\t'}in the middle of this string
                Tab at the end of this line.${'\t'}
                """.trimIndent()

KDOC indentation

            class Foo {
                  /**
                    *
                     */
                fun foo() {}
            }

Improperly indented multiline strings:

            fun foo() {
              println("""
                line1
                    line2
                    """.trimIndent())
            }

            fun foo() {
                println("""
                    line1
                line2""".trimIndent())
            }

            fun foo() {
                println(
                    """
                """.trimIndent()
                )
            }

            fun foo() {
                println(
                    """
                text ""

                     text
                     ""
                """
                )
            }

Multiline string as property value:

            class C {
                val CONFIG_COMPACT1 = """
                        {
                        }
                    """.trimIndent()
                val CONFIG_COMPACT2 = // comment
                    """
                        {
                        }
                    """.trimIndent()
            }

Report (no autocorrect is possible) in case indentation of multiline string contains tabs and spaces like in example below:

                val foo = """
                      line1
                ${'\t'}line2
                    """.trimIndent()

Fix indentation of multiline string in case all lines in it are indented with the wrong indentation character only (or are blank):

            val str =
                """
            ${'\t'}line1
            ${'\t'}${'\t'}line2
                """.trimIndent()

Note: the lint-checking currently fails on file "IndentationRuleTrimIndentTest" only because the directive that disable the indent-rule from being executed on that file needs to be merged first (see this pull request.

Paul Dingemans added 9 commits January 3, 2021 16:26
* 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
SplitString is less intuitive compared to a destructure declaration.
The call splitIndentAt guarantees that the indent only contains
whitespace characters.
# Conflicts:
#	ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRule.kt
@romtsn
Copy link
Collaborator

romtsn commented Jan 9, 2021

The PR is aiming to fix multiple things and is also very hard to review in its current form - could you split it into small logical chunks?

@paul-dingemans
Copy link
Collaborator Author

I was afraid of that remark but I do understand it. I will give it a try and I am sure that I can split off some minor changes. But the core change of the indentRule, e.g. method indentStringTemplate will still be a complex and very intertwined change. It would be great if you in mean time can merge PR fix ktlint-disable as it will be blocking for core problem as well.

@Tapchicoma
Copy link
Collaborator

Regarding #1038 - I wanted to check it in a few days. Probably the logic there need to revised and have more clear use-cases.

@paul-dingemans
Copy link
Collaborator Author

I was afraid of that remark but I do understand it. I will give it a try and I am sure that I can split off some minor changes. But the core change of the indentRule, e.g. method indentStringTemplate will still be a complex and very intertwined change. It would be great if you in mean time can merge PR fix ktlint-disable as it will be blocking for core problem as well.

Part 1: fix indentation multiline string

@paul-dingemans
Copy link
Collaborator Author

I am not sure how to proceed with the other parts. Some others part rely on part 1 to be merged with master. Should I wait till part 1 is merged to master? Or can I also submit a PR is branched off on top of part 1?

@romtsn
Copy link
Collaborator

romtsn commented Jan 9, 2021

Yeah you can submit a PR branched off of part 1 and also target part 1

@paul-dingemans
Copy link
Collaborator Author

This is my first open source project to which I am contributing. I do not understand how to create the PR for part2.

On my local machine I have created a new branch 'multiline-string-indent-2' based on top of branch 'multiline-string-indent' and pushed those changes to my fork (https://github.com/paul-dingemans/ktlint/branches). When I am trying to create a PR in the master repository, I can not indicate that this PR should be compared with the 'multiline-string-indent' branch. See image. I do not want to use master as base because this PR should not be merged until part is merged.

I hope you can explain what I should do. Tnx in advance.

@romtsn
Copy link
Collaborator

romtsn commented Jan 10, 2021

I am sorry for misinforming you, you obviously can do it only on your own fork and not with the base ktlint repo. Then the only way would be to wait until we merge your first PR and then rebase and open following PRs on top

@paul-dingemans
Copy link
Collaborator Author

I am sorry for misinforming you, you obviously can do it only on your own fork and not with the base ktlint repo. Then the only way would be to wait until we merge your first PR and then rebase and open following PRs on top

That is no problem. All followup PR's are ready to be pushed once the first part is merged. It would be really great if you can out that into motion.

@paul-dingemans
Copy link
Collaborator Author

Closing this PR while awaiting merge of #1052

@paul-dingemans paul-dingemans deleted the 575-indentation-rule-fixes branch October 30, 2021 10:10
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.

3 participants