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

Respect @Suppress("RemoveCurlyBracesFromTemplate") #157

Closed
JLLeitschuh opened this issue Feb 22, 2018 · 12 comments
Closed

Respect @Suppress("RemoveCurlyBracesFromTemplate") #157

JLLeitschuh opened this issue Feb 22, 2018 · 12 comments
Labels

Comments

@JLLeitschuh
Copy link
Contributor

JLLeitschuh commented Feb 22, 2018

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
Copy link
Contributor

Can I try to implement this one?

@shyiko
Copy link
Collaborator

shyiko commented Mar 30, 2018

@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
Copy link
Contributor

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

@shyiko
Copy link
Collaborator

shyiko commented Mar 30, 2018

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

NOTE: @Suppress can be applied pretty much anywhere.

@arturbosch
Copy link
Contributor

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

@AleksandrSl
Copy link
Contributor

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

@AleksandrSl
Copy link
Contributor

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
Copy link
Collaborator

shyiko commented May 27, 2018

I'd go with AleksandrSl#2 (only it should be it.toSet() instead of setOf() if I'm reading it right).

@blcooley
Copy link

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

@AleksandrSl
Copy link
Contributor

@blcooley It was merged only in my fork) Here #263 is not merged yet. @shyiko what should we do with it?

@Raibaz
Copy link
Contributor

Raibaz commented Oct 24, 2019

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

@Tapchicoma Tapchicoma added the bug label Oct 29, 2019
Tapchicoma pushed a commit that referenced this issue Nov 11, 2019
Adds a way to respect standard suppress code style annotations.
@Tapchicoma
Copy link
Collaborator

PR with support was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants