-
Notifications
You must be signed in to change notification settings - Fork 503
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
Suppress remove curly braces from template #157 #263
Suppress remove curly braces from template #157 #263
Conversation
Hi! What about this one, should I modify something? |
Hi @shashachu, is this issue still needed to be done? If so I can resolve merge conflicts, otherwise I think it's time to close PR and issue) |
Hi, sorry about that. Can you resolve the conflicts and then I or someone else here will take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good after conflicts are resolved
ktlint-ruleset-standard/src/test/resources/spec/string-template/lint.kt.spec
Outdated
Show resolved
Hide resolved
611efb7
to
c66826e
Compare
@NightlyNexus I think all work is done, any other comments? |
ktlint-ruleset-standard/src/test/resources/spec/string-template/lint.kt.spec
Outdated
Show resolved
Hide resolved
Hi, sorry to bother you again) Is there a reason that prevents PR from being merged? |
Hi! No, this PR just fell through the cracks. |
Can you rebase and just make sure the build still passes? I can merge after. |
fa3f64b
to
68866bd
Compare
@@ -490,6 +515,12 @@ object KtLint { | |||
comment.replace(Regex("\\s"), " ").replace(" {2,}", " ").split(" ") | |||
|
|||
private fun <T> List<T>.tail() = this.subList(1, this.size) | |||
|
|||
private val suppressAnnotationRuleMap = mapOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this map could be moved to some config file, to ease addition of new suppression
rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about having this mapping:
- "RemoveCurlyBracesFromTemplate" just becomes an alias to "string-template" rule
- This alias is detached from the rule itself
- Overall this PR goes into direction of adding another way for suppress rules
To be consistent, I would propose to use following approach:
@Suppress("ktlint-string-template")
to disable rule for the block of code and remove this map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this map looks weird, however original purpose for this PR was to respect annotations like @Suppress("RemoveCurlyBracesFromTemplate")
to disable both IntellijIDEA and ktlint checks at the same time. Otherwise we will have to add both annotations
@Suppress("ktlint-string-template")
@Suppress("RemoveCurlyBracesFromTemplate")
println("Not found: ${notFound}")
and this way all sense in introduction of support for @Suppress
annotation seems lost to me, cause it becomes specific for ktlint, and as you mentioned we already have ways to suppress ktlint checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this map looks weird, however original purpose for this PR was to respect annotations like @Suppress("RemoveCurlyBracesFromTemplate") to disable both IntellijIDEA and ktlint checks at the same time.
Sorry, missed that. Then, as intermediate step, having this map looks ok. In the future would be better that rule provides a list of suppress names aliases.
Done! |
// Extract all Suppress annotations and create SuppressionHints | ||
val psi = node.psi | ||
if (psi is KtAnnotated) { | ||
psi.annotationEntries | ||
.filter { | ||
it.calleeExpression?.constructorReferenceExpression | ||
?.getReferencedName() in suppressAnnotations | ||
}.flatMap(KtAnnotationEntry::getValueArguments) | ||
.mapNotNull { it.getArgumentExpression()?.text?.removeSurrounding("\"") } | ||
.mapNotNull(suppressAnnotationRuleMap::get) | ||
.let { suppressedRules -> | ||
if (suppressedRules.isNotEmpty()) { | ||
result.add( | ||
SuppressionHint( | ||
IntRange(psi.startOffset, psi.endOffset), | ||
suppressedRules.toSet() | ||
) | ||
) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you move this to some private method for better readability?
@@ -490,6 +515,12 @@ object KtLint { | |||
comment.replace(Regex("\\s"), " ").replace(" {2,}", " ").split(" ") | |||
|
|||
private fun <T> List<T>.tail() = this.subList(1, this.size) | |||
|
|||
private val suppressAnnotationRuleMap = mapOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about having this mapping:
- "RemoveCurlyBracesFromTemplate" just becomes an alias to "string-template" rule
- This alias is detached from the rule itself
- Overall this PR goes into direction of adding another way for suppress rules
To be consistent, I would propose to use following approach:
@Suppress("ktlint-string-template")
to disable rule for the block of code and remove this map.
Current flow is that all string-template errors are suppressed by RemoveCurlyBracesFromTemplate.
It turned out that @Suppress with arrayOf and [] are compile errors, and can be used only with destructuring.
Add more test for different annotations use sites.
Shame on me, forgot to remove "debugging" code
Distinct is redundant, cause toSet is called on the list afterwards.
68866bd
to
4ee8253
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
No description provided.