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
Fix issue with wrong report name #1145
Fix issue with wrong report name #1145
Conversation
When splitting a report path provided on JCommander, given a Windows path would cause a false split, that would continue the execution despite the assert check. This commit fixes this issue by the assumption that an absolute path on Windows systems, will contain the current disk drive's letter. Resolves: detekt#1123 - detekt#1141
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.
Instead of checking for the size only we should assert a bit more to verify that what we do is actually correct. In this case asserting the paths
@vanniktech can you elaborate on that? I am afraid I did not get what you mean :P |
@pavlospt I think we should check the entries of the report for equality, not just checking the size. This caused the bug in the first place. @vanniktech please correct me if you have other intentions. it("should parse multiple report entries") {
assertThat(reports).hasSize(4)
} |
@schalkms sure, i can update the test to assert on that! |
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.
Thanks for looking into this!
@schalkms @vanniktech I enhanced |
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.
Thanks for fixing this so quickly. Looks good!
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.
Yup that's what I meant
@@ -11,10 +11,25 @@ import java.nio.file.Paths | |||
*/ | |||
data class ReportPath(val kind: String, val path: Path) { | |||
companion object { | |||
private const val NUM_OF_PARTS_UNIX = 2 | |||
private const val NUM_OF_PARTS_WINDOWS = 3 |
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.
waaah, jupp I never use windows >.<
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.
Thanks for the fix!
When splitting a report path provided on JCommander, given a Windows path
would cause a false split, that would continue the execution despite the assert check.
This commit fixes this issue by the assumption that an absolute path on Windows
systems, will contain the current disk drive's letter.
Resolves: #1123 - #1141