Fix gocov formatting issue #479 #480
Conversation
Fixes issue codeclimate#479 The formatter used to assume that the number of statements always equals the number of lines in a block. This is not true when there are blank lines for example. This change colours by line number rather than by statement. Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com> Co-authored-by: Danail Branekov <danailster@gmail.com>
|
|
| @@ -70,14 +69,15 @@ func (r Formatter) Format() (formatters.Report, error) { | |||
| } | |||
| blocks[lstIdx].Count += b.Count | |||
| } | |||
| lineNum := 1 | |||
There was a problem hiding this comment.
can't start lines be 0 ? 🤔
There was a problem hiding this comment.
They used to, but we changed them to be 1-based for better readability (lines in an editor start from 1 usually). This way for lineNum < b.StartLine reads like "do this and that for all lines before start of block". If lines are zero-based the statement becomes for lineNum < b.StartLine - 1 which at least for me is harder to read. But I can change it to start from zero if you prefer it that way :).
There was a problem hiding this comment.
oh, I see. Thanks for the explanation. However, there might be folks out there using the test-reportoer downloading it's latest version. If they haven't upgraded the gocov version they are using, things will start to behave differently for them once we merge this. Could we make it to be for lineNum < b.StartLine - 1 for this reason ? I mean, for backwards compatibility.
There was a problem hiding this comment.
It is actually backwards compatible this way. Iterating from 0 to startLine - 1 is effectively the same as iterating from 1 to startLine. The index itself is not used anywhere, but is just a counter, so it cannot affect anything user facing.
There was a problem hiding this comment.
@georgethebeatle thanks for the contribution, this is a nice catch 👏🏼 . Sorry took us some time to review, we have been busy these days.
Only left a minor comment.
|
@georgethebeatle this has been release already, it would be great if you could confirm that it has addressed the issue that you reported successfully 👍🏼 |
|
Thanks for the quick reaction @fede-moya. The release behaves as expected and we are already using it! |
|
@georgethebeatle ooh great - Thanks for sharing that 🙌🏼 . Regards! |
This PR fixes issue #479
The formatter used to assume that the number of statements always equals
the number of lines in a block. This is not true when there are blank
lines for example. This change colours by line number rather than by
statement.