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

Add new experimental rule chain-method-continuation #2088

Merged

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented Jun 24, 2023

Description

Add ChainMethodContinuation(chain-method-continuation) rule

This rule ensures chain operators(. or ?.) aligns with next call
When there is } in the new line this also ensures that the chain operators align with } along with the next call method

This partially fixes #1953

Checklist

  • PR description added
  • tests are added
  • KtLint has been applied on source code itself and violations are fixed
  • documentation is updated
  • CHANGELOG.md is updated

In case of adding a new rule:

@paul-dingemans
Copy link
Collaborator

Formatting code below:

val bar =
    listOf(1, 2, 3). // xxx
        map {
            it * it
        }

results in code which can not be compiled:

val bar =
    listOf(1, 2, 3)
        .// xxxmap {
            it * it
        }

@atulgpt atulgpt force-pushed the fixes/1953/chained-method-continuation branch from 2cb679e to 4842266 Compare July 25, 2023 20:27
@atulgpt atulgpt closed this Jul 25, 2023
@atulgpt atulgpt reopened this Jul 25, 2023
@atulgpt atulgpt force-pushed the fixes/1953/chained-method-continuation branch 2 times, most recently from 038746c to dec3753 Compare July 25, 2023 20:37
This rule ensures chain operators(`.` or `?.`) aligns with next call
When there is `}` in the new line this also ensures that the chain
operators align with `}` along with next call method
Handle minor comments
Update docs and remove release docs
@atulgpt atulgpt force-pushed the fixes/1953/chained-method-continuation branch from dec3753 to baef5ba Compare August 1, 2023 16:28
Remove the usage of `noNewLineInOpenRange`
@atulgpt atulgpt force-pushed the fixes/1953/chained-method-continuation branch from 7b2f9af to 41c1721 Compare August 2, 2023 09:48
atulgpt and others added 12 commits August 4, 2023 17:35
- Fix offsets
- Messages only chain operator
- if chain operator is at end of line then two messages needs to be emitted. One message because as a whitespace was expected before the chain operator. The other message because no whitespace was expected after the chain operator.
- Break chained methods when max line is exceeded
- A chained method should not continue on the previous line if that ends with "!!"

Add discourage locations for comments between chain operator and the expression. Comments before the chain operator have to be allowed.
…rces the chain method to be wrapped even when the maximum line length is not exceeded
- Rename ChainMethodContinuation to ChainMethodContinuationRule
- Add new function replaceStringTemplatePlaceholder in TestLiterals
# Conflicts:
#	CHANGELOG.md
#	documentation/release-latest/docs/rules/experimental.md
.filter {
listOf(1, 2, 3).map { it * it }.size > 1
}!!
.map {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't this be as below

}!!.map {

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it disaligns filter and map too much. At least, I found it very distracting.

""".trimIndent()
val formattedCode =
"""
val foo = listOf(1, 2, 3)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we also test nested chains? for example

val foo = listOf(1, 2, 3).foo1(listOf(1, 2, 3).foo1().foo2().foo3().foo4().foo5().foo6()).foo2().foo3().foo4().foo5().foo6()

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already catered for. Above is formatted as:

val foo =
    listOf(1, 2, 3)
        .foo1(
            listOf(1, 2, 3)
                .foo1()
                .foo2()
                .foo3()
                .foo4()
                .foo5()
                .foo6(),
        ).foo2()
        .foo3()
        .foo4()
        .foo5()
        .foo6()

}

@Test
fun `Given nested chains which are correctly wrapped`() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we also add TC with index operator like below

list(1)[
    0
]
.inc()

list(1)[
    0
].
inc()

list(1)[
    0
]!!
.inc()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, this is not handled well (yet). It is formatted as:

    list(1)[
        0,
    ]
        .inc()

    list(1)[
        0,
    ]
        ?.inc() // Assumption is that here the `?.` chain operator was intended

    list(1)[
        0,
    ]!!
        .inc()

So a TC and fix is needed. I would say that expected outcome should be:

    list(1)[
        0,
    ].inc()

    list(1)[
        0,
    ]?.inc() // Assumption is that here the `?.` chain operator was intended

    list(1)[
        0,
    ]!!
        .inc()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Array access expressions are now added.

@paul-dingemans paul-dingemans merged commit 26e2837 into pinterest:master Aug 15, 2023
21 checks passed
@paul-dingemans paul-dingemans changed the title Fixes/1953/chained method continuation Add new experimental rule chain-method-continuation Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrap chained calls
2 participants