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.

Sign up for GitHub

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

Jump to bottom

Fixes/1078/statement wrap after semi #2065

Merged
merged 7 commits into from
Jun 5, 2023
Merged

Fixes/1078/statement wrap after semi #2065

merged 7 commits into from
Jun 5, 2023

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented May 31, 2023 •
edited
Loading

Description

Add the SEMICOLON branch in WrappingRule and insert a newline after the semicolon

Fixes: #1078

Checklist

  • PR description added
  • tests are added
  • KtLint has been applied on source code itself and violations are fixed
  • documentation is updated: NA as no new rule is added
  • CHANGELOG.md is updated

@atulgpt atulgpt force-pushed the fixes/1078/statement-wrap-after-semi branch from 0ee341d to 72b6361 Compare May 31, 2023 18:04
@atulgpt
Handle enum entry with semicolon
ba9b023 
Add multiple entries, enums and assertions
paul-dingemans
paul-dingemans requested changes Jun 1, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/ReporterAggregator.kt Show resolved Hide resolved
...ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRule.kt Show resolved Hide resolved
...ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRule.kt Show resolved Hide resolved
...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRuleTest.kt Outdated Show resolved Hide resolved
...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRuleTest.kt Outdated Show resolved Hide resolved
...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRuleTest.kt Outdated Show resolved Hide resolved
...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRuleTest.kt Outdated
val lambda: () -> Unit = { }
}
""".trimIndent()
wrappingRuleAssertThat(code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In tests like this also add .addAdditionalRuleProvider { NoSemicolonsRule() } so that the formatted result also has cleaned up the unwanted semicolon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But as per the doc of addAdditionalRuleProvider, it runs before the rule which is getting unit tested An additional rule provider for a rule which actually is executed before the rule under test,

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does not always lead to problems. You can always try to add it anyways and document it like below:

                .hasLintViolations(
                    // Some violations below are only reported during linting but not on formatting. After wrapping the
                    // nullable type, there is no need to wrap the parameters.
                    LintViolation(2, 12, "Expected new line before function type as it does not fit on a single line"),
                    LintViolation(2, 13, "Parameter should start on a newline"),
                    LintViolation(2, 24, "Parameter should start on a newline"),
                    LintViolation(2, 35, "Parameter should start on a newline"),
                    LintViolation(2, 44, "Missing newline before \")\""),
                    LintViolation(2, 53, "Expected new line after function type as it does not fit on a single line"),
                ).isFormattedAs(formattedCode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added addAdditionalRuleProvider and it really cleaned semicolons 😅 It seems even after An additional rule provider for a rule which actually is executed before the rule under test, due to constraints NoSemicolonsRule make sure that it is run after the WrappingRuleTest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I have not added addAdditionalRuleProvider as I also wanted to test this rule w/o NoSemicolonsRule

Copy link
Collaborator

Choose a reason for hiding this comment

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

Downside is that it is now sometime confusing why the NoSemicolonsRule is not present in some test while it is added in other tests. As the default behavior of ktlint is to run both rules, I prefer to add the rule because the formattedCode in that case looks closest to the normal behavior of ktlint.
But when there is a specific need in a test to also validate the end result in case the rule is disabled, then I would suggest to make it explicit in the name of the test or with a comment why the the NoSemicolonsRule is not added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this makes more sense. I have added one TC Given two variables run without NoSemicolonsRule

...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRuleTest.kt Outdated Show resolved Hide resolved
...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRuleTest.kt Outdated Show resolved Hide resolved
atulgpt added 2 commits June 3, 2023 03:39
@atulgpt
Correct CHANGELOG.md messaging
e518366 
Split TCs into smaller ones
Rename `insertNewLineBeforeSemi` to `insertNewLineAfterSemi`
@atulgpt
Add addAdditionalRuleProvider { NoSemicolonsRule() }
8c84e2a
paul-dingemans
paul-dingemans requested changes Jun 3, 2023
View reviewed changes
Copy link
Collaborator

@paul-dingemans paul-dingemans left a comment

Choose a reason for hiding this comment

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

Tnx for addessing the review comments. The tests are way more readable now. I adressed one more issue.

...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/WrappingRuleTest.kt Outdated Show resolved Hide resolved
@atulgpt
Add addAdditionalRuleProvider in every TC
384e92e 
Create a TC w/o `addAdditionalRuleProvider`
Rename `INDEX` enum to `FOO` enums
@paul-dingemans paul-dingemans merged commit ff940d3 into pinterest:master Jun 5, 2023
@paul-dingemans
Copy link
Collaborator

Tnx for the contributation @atulgpt !

@atulgpt atulgpt deleted the fixes/1078/statement-wrap-after-semi branch June 5, 2023 19:49
@paul-dingemans paul-dingemans added this to the 0.50.0 milestone Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-dingemans paul-dingemans paul-dingemans requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.50.0
Development

Successfully merging this pull request may close these issues.

Prevent multiple statements on the same line
2 participants
@atulgpt @paul-dingemans