pinterest / ktlint

An anti-bikeshedding Kotlin linter with built-in formatter
https://pinterest.github.io/ktlint/
MIT License
6.23k stars 512 forks source link

Respect @Suppress("RemoveCurlyBracesFromTemplate") #157

Closed JLLeitschuh closed 5 years ago

JLLeitschuh commented 6 years ago

If I have some code like this:

val prettyString = computeSomeString()

@Suppress("RemoveCurlyBracesFromTemplate")
val expectedPrint = """
|${TAB}==========================================================
|${TAB}=                     GRAPH OVERVIEW                     =
|${TAB}==========================================================
|${TAB}Nodes: 2
|${TAB}Links: 2
|${TAB}
|${TAB}Link Breakdown:
|${TAB}${TAB}[Aout1->Bin1]; Weight: 1.0, Capacity: 10000.0
|${TAB}${TAB}[Bout1->Ain1]; Weight: 1.0, Capacity: 10000.0
""".trimMargin()

assertEquals(expectedPrint, prettyString)

Ktlint reformats it to this:

@Suppress("RemoveCurlyBracesFromTemplate")
val expectedPrint = """
|$TAB==========================================================
|$TAB=                     GRAPH OVERVIEW                     =
|$TAB==========================================================
|${TAB}Nodes: 2
|${TAB}Links: 2
|$TAB
|${TAB}Link Breakdown:
|${TAB}$TAB[Aout1->Bin1]; Weight: 1.0, Capacity: 10000.0
|${TAB}$TAB[Bout1->Ain1]; Weight: 1.0, Capacity: 10000.0
""".trimMargin()

The reason that I've put on the @Suppress("RemoveCurlyBracesFromTemplate") is because I want to have the "fake" indentation to be consistent. As you can see the character count between $TAB is off by two characters from ${TAB}. I'm going for readability in my test over what is the "most correct".

AleksandrSl commented 6 years ago

Can I try to implement this one?

shyiko commented 6 years ago

@AleksandrSl That would be great! As a starting point - take a look at https://github.com/shyiko/ktlint/blob/master/ktlint-core/src/main/kotlin/com/github/shyiko/ktlint/core/KtLint.kt#L397.

I'm not sure how to map compiler warnings to rule ids, though (RemoveCurlyBracesFromTemplate to string-template in this case; note that these two have different scope so in order to support @Suppress rules will need to have access to suppression ranges complicating the matter).

Current workaround is to use /* ktlint-disable string-template */.

P.S. If you feel like helping adding missing https://kotlinlang.org/docs/reference/coding-conventions.html & https://android.github.io/kotlin-guides/style.html rules is of (much) greater value then fixing this particular issue.

AleksandrSl commented 6 years ago

@shyiko I thought to handle suppress annotation directly in this rule, however your suggestion seems more natural

shyiko commented 6 years ago

@AleksandrSl I guess that would be OK too (+ easier to implement).

NOTE: @Suppress can be applied pretty much anywhere.

arturbosch commented 6 years ago

@AleksandrSl you can check out how we suppress issues by annotations @detekt, maybe it helps.

AleksandrSl commented 6 years ago

@shyiko Hi, I've come up with two draft implementations https://github.com/AleksandrSl/ktlint/pull/2 - this one acts by adding SupressHints. Disadvantage - we suppress the whole string-template rule. https://github.com/AleksandrSl/ktlint/pull/1 - this one acts in string-template rule. Disadvantage - we should scan all annotated parents of each node.

AleksandrSl commented 6 years ago

Hi @shyiko, did you have time to see proposed implementations, or this issue will be resolved by global changes in the way ktlint works?

shyiko commented 6 years ago

I'd go with https://github.com/AleksandrSl/ktlint/pull/2 (only it should be it.toSet() instead of setOf() if I'm reading it right).

blcooley commented 5 years ago

Doing some triage, it looks like the PR AleksandrSI#2 has been merged. Can this issue be closed @shyiko @AleksandrSl ?

AleksandrSl commented 5 years ago

@blcooley It was merged only in my fork) Here https://github.com/shyiko/ktlint/pull/263 is not merged yet. @shyiko what should we do with it?

Raibaz commented 5 years ago

Doing some triage, looks like #263 can be merged and this issue closed, @shyiko WDYT?

Tapchicoma commented 5 years ago

PR with support was merged.