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
Implement MultilineRawStringIndentation
#5058
Conversation
5ff5f44
to
5f239fc
Compare
5f239fc
to
37ac386
Compare
@BraisGabin did you mix up the changes required for this new rule with another branch? In particular, this PR seems to include too much. |
37ac386
to
27b256c
Compare
Yes, I had one extra commit (the squash strategy doesn't work too good with chained PRs). Probably this PR could be reviewed commit by commit. There are two prior refactors that I needed: One to be able to tests this PR properly and other to reduce the duplicated code. I can split this PR if you feel it will make it easier to review. |
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 will finish tomorrow. Just a few comments now ;)
...src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentationSpec.kt
Outdated
Show resolved
Hide resolved
...yle/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentation.kt
Outdated
Show resolved
Hide resolved
...yle/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentation.kt
Outdated
Show resolved
Hide resolved
...yle/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentation.kt
Outdated
Show resolved
Hide resolved
...src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentationSpec.kt
Show resolved
Hide resolved
...yle/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentation.kt
Show resolved
Hide resolved
...yle/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentation.kt
Outdated
Show resolved
Hide resolved
.onEach { (lineNumber, line, currentIndent) -> | ||
val location = containingFile.getLocation( | ||
SourceLocation(lineNumber, if (line.isBlank()) 1 else currentIndent + 1), | ||
SourceLocation(lineNumber, line.length + 1) | ||
) | ||
|
||
report(this, location, message(desiredIndent, currentIndent)) |
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.
Do we actually want to report one violation for each line that is incorrectly indented? Consider a code block like in detekt tests. If you get the indentation wrong, it is most likely wrong for entire block. Would it not be enough to mark the multi line string as a violation as a whole? That most likely would make the rule much easier to implement.
Specifically I do not like the fact that
val a = """
Hello world!
How are you?
""".trimIndent()
produces 2 violations, while
val a = """
Hello world!
How are you?
""".trimIndent()
produces one
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.
Yes... this was a difficult decission. I was thinking about a long raw string with only one/few lines with the wrong identation. In that case I wanted to point out exactly which line was the wrong one. The problem is that if all of them are wrong I end up raising all of them
The second case was easy because if it happens I know that all the lines are wrong so I just raise the error once.
I feel that this is a tradeoff:
- If I raise only one the cases where all of them are wrong will be easier,
- But if only some of them are wrong this approuch would be better.
🤷 I'm fine with both of them to be honest.
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 see. I think the code could have been easier to write and to maintain if it were just one violation per multi line string, but I see your point.
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'm merging it like this. We can always revisit this once we get the users feedback.
...yle/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentation.kt
Outdated
Show resolved
Hide resolved
...src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineRawStringIndentationSpec.kt
Outdated
Show resolved
Hide resolved
487cee5
to
956b99d
Compare
@marschwar Thank you so much for the this great review :). I fix some of your comments and answer others. I'm not against the changes that you point our in the comments were I answered I just wanted to share the why behind that code/decission. If you still think that it would be beneficial to change that part of the code I will change it :) |
Codecov Report
@@ Coverage Diff @@
## main #5058 +/- ##
=========================================
Coverage 84.91% 84.91%
Complexity 3614 3614
=========================================
Files 502 502
Lines 11887 11887
Branches 2237 2237
=========================================
Hits 10094 10094
Misses 690 690
Partials 1103 1103 Continue to review full report at Codecov.
|
Same here. I do not feel strongly enough about them but wanted to discuss them with you. |
78ddade
to
6245074
Compare
956b99d
to
b115929
Compare
…/rules/style/MultilineRawStringIndentation.kt Co-authored-by: marschwar <marschwar@users.noreply.github.com>
…/rules/style/MultilineRawStringIndentation.kt Co-authored-by: marschwar <marschwar@users.noreply.github.com>
…/rules/style/MultilineRawStringIndentation.kt Co-authored-by: marschwar <marschwar@users.noreply.github.com>
…/rules/style/MultilineRawStringIndentation.kt Co-authored-by: marschwar <marschwar@users.noreply.github.com>
b115929
to
ab9ab07
Compare
This implements one of the proposed rules in #4705
To be more precise it implements this one:
This rule was a complex one. Format is hard >_<.