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

Remove lint warning about unexpected indentation outside of indent rule #1284

Merged

Conversation

paul-dingemans
Copy link
Collaborator

@paul-dingemans paul-dingemans commented Nov 10, 2021

Description

Only the indentation rule should emit warnings about incorrect indentation to avoid
conflicting warnings from different rules about the indentation of the exact same
line. However, those other rules should still fix the indentation as good as they
can for the case that the indent rule is not run at all.

Closes #1267, #1119, #1045, #1255, #1319, #1320, #897, #1337

Checklist

  • tests are added
  • CHANGELOG.md is updated

Paul Dingemans added 2 commits November 10, 2021 20:58
Only the indentation rule should emit warnings about incorrect indentation to avoid
conflicting warnings from different rules about the indentation of the exact same
line. However, those other rules should still fix the indentation as good as they
can for the case that the indent rule is not run at all.

Closes pinterest#1267, pinterest#1119, pinterest#1045
} else {
emit(child.startOffset, errorMessage(child), true)
}
if (autoCorrect) {
val finalIndent =
(if (cut > -1) spacing.substring(0, cut) else "") + intendedIndent
// Although no warning is displayed about incorrect indentation, an attempt is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strange that we wouldn't emit a warning on linting, but we'd change indentation on formatting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is a bit unusual. But look at the number of issue we get about those warnings. So the warning has to be removed at minimum. Keeping the indent fix during format only has value in case the indent rule is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't have different behavior for linting vs formatting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with removing the functionality to fix indentation inside the argument/parameter list wrapping rule in order to make linting and formatting consistent again. Some users might consider this a breaking change.

@shashachu please confirm whether that is the way to go.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let's disable autocorrection of indentation for now

paul-dingemans pushed a commit to paul-dingemans/ktlint that referenced this pull request Dec 12, 2021
It is not yet possible to enable the rule as some violations are actually false
positives. This will be solved by pinterest#1284
@paul-dingemans
Copy link
Collaborator Author

Like before a warning is emitted in case the current indentation is not as expected. However, the emitted warning now no longer reports the expected indentation but just that the argument/parameter should be on a separate line unless all of them fit on a single line.

@romtsn
Copy link
Collaborator

romtsn commented Jan 20, 2022

hm, can we remove autocorrection of indentation entirely from these rules? let's just delegate this to the indent-rule completely and say that we do not support indentation alignment in these wrapping rules (even the names suggest that they are wrapping and not indenting :))

romtsn added a commit that referenced this pull request Jan 21, 2022
* Add separate verification build step to include experimental rules

Ktlint should apply the dogfooding principal and only provide experimental
rules that at least on the ktlint code base itself gives satisfiable
results.

Initially all experimental rules that cause violations have been disabled,
so that a separate commit can be created to enable each specific rule.

* Enable rule experimental:spacing-between-declarations-with-comments

For BaselineTests it was necessary to rename the files which are used for testing
to have a non standard kotlin file extension. This prevents the files from being
changed when running the ktlint formatting on the ktlint code base itself. Note
that the baseline protection mechanism did work in this case and as of that has
been removed from the command.

* Enable rule experimental:no-empty-first-line-in-method-block

* Enable rule experimental:annotation

* Resolve some violations of rule experimental:argument-list-wrapping

It is not yet possible to enable the rule as some violations are actually false
positives. This will be solved by #1284

* Enable rule experimental:spacing-between-declarations-with-annotations

* Enable rule experimental:trailing-comma

For now, it has been chosen to disallow trailing comma's instead of forcing them to
be added. Reasons for this are two folded. The number of changes is considerably
smaller. More importantly is that the benefit, with respect to avoiding future merge
conflicts, seems not that big when scanning the code change which would result from
forcing the trailing comma to be added.

* Update changelog

* Remove autocorrect mode from build step ktlint_experimental

* Run the experimental rules by default

There is no need for a separate build task to run the experimental rules. The
experimental rules can be executed by default in the "ktlint" task. Also, the
baseline has been fixed so there is no longer a need to use extension "_kt"
for the baseline test files.

Closes #1222

* Update ktlint/src/test/resources/TestBaselineFile.kt

* Update ktlint/src/test/resources/TestBaselineFile.kt

* Update ktlint/src/test/resources/TestBaselineExtraErrorFile.kt

* Update ktlint/src/test/resources/TestBaselineExtraErrorFile.kt

* Revert BaselineTests

* Revert ktlint-test-baseline

* Fix tests

* Exclude all test resources from the ktlint module from the linting task

Those source files all contain linting errors which have to be reported
by unit tests. Therefore they should not be reported during a normal
build because those should not be fixed as that would result in the
tests to fail.

* Revert renaming of file test-baseline.xml

* Fix lint errors due to merge of master in branch

Co-authored-by: Paul Dingemans <pdingemans@bol.com>
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
@paul-dingemans
Copy link
Collaborator Author

hm, can we remove autocorrection of indentation entirely from these rules? let's just delegate this to the indent-rule completely and say that we do not support indentation alignment in these wrapping rules (even the names suggest that they are wrapping and not indenting :))

I think this would give a bad user experience in case the indent rule would be disabled. More importantly is that it would have a big Ktlint-developer impact as this would also imply that the expected formatted code in unit tests would have to be de-indented. So, writing a test like below:

    @Test
    fun testFormatArgumentsWithNestedCalls() {
        assertThat(
            ArgumentListWrappingRule().format(
                """
                val x = test(
                    one("a", "b",
                    "c"),
                    "Two", "Three", "Four"
                )
                """.trimIndent()
            )
        ).isEqualTo(
            """
            one(
            "a",
            "b",
            "c"
            ),
            "Two",
            "Three",
            "Four"
            )
            """.trimIndent()
        )
    }

@romtsn
Copy link
Collaborator

romtsn commented Jan 26, 2022

gotcha, let's merge it like that then!

Paul Dingemans added 2 commits January 28, 2022 19:15
Unit tests for experimental rules are allowed to depend on standard rules. Unit
tests for standard rules may not depend on experimental rules.
… argument already contains a newline

Fixing incorrect indents is the sole responsibility on the IndentationRule. The
ArgumentListWrappingRule and ParameterListWrappingRule should only consider
wrapping issues.
@paul-dingemans
Copy link
Collaborator Author

When checking all reported issues, I encountered a problem which I did not see before. The incorrect indentation warning was sometimes replace with another incorrect warning. No I have changed the rules to ignore the indentation if the the whitespace before the parameter, argument or closing parathesis contains a new line because this means that the wrapping was done correctly. Previously an incorrect warning was shown in case that whitespace contained a newline but was incorrectly indented according to that rule.

In order to keep the unit tests of the rules readable i have modified the lint/format utilities to accept a list of rules instead of a single rule. In this way, I can execute both the wrapping as well as the indentation rule together. I am going to need that functionality also for the new signature rewrite rule that I am developing, so I think it is a good addition to our testing toolkit.

I have, once more checked that all issues associated with this PR are now indedd resolved. If the build succeeds, I will merge to master.

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.

Split type arguments break 0.43.0
3 participants