fix: readability improvements to artifact size metrics #41
Conversation
|
The new format for the comments: Affected ArtifactsSignificantly increased in size
Changed in size
|
| @@ -109,34 +110,73 @@ internal abstract class AnalyzeArtifactSizeMetrics : DefaultTask() { | |||
| return ArtifactSizeMetricsAnalysis(artifactSizeMetrics, significantChange, changeHappened) | |||
| } | |||
|
|
|||
| // There are small fluctuations in artifact size that are not real delta | |||
| private fun Long.isNotaFluctuation() = abs(this) > 5L | |||
There was a problem hiding this comment.
question: Is this number big enough? In your example comment, there was a delta of 10 bytes that also seems like a fluctuation.
There was a problem hiding this comment.
I'm basing it off the fluctuations seen here: awslabs/aws-sdk-kotlin#1291. They're all less than 5. That 10 I added manually but I'm not opposed to upping the threshold
There was a problem hiding this comment.
Ah ok, that seems fine then. I didn't realize the 10 was a manual data point
There was a problem hiding this comment.
If we're hiding the unimportant/no-action-required changes by default, do we still need to filter out these fluctuations?
There was a problem hiding this comment.
It's not really necessary but I also wouldn't consider it real delta
There was a problem hiding this comment.
It pollutes an already large table
| @@ -109,34 +110,73 @@ internal abstract class AnalyzeArtifactSizeMetrics : DefaultTask() { | |||
| return ArtifactSizeMetricsAnalysis(artifactSizeMetrics, significantChange, changeHappened) | |||
| } | |||
|
|
|||
| // There are small fluctuations in artifact size that are not real delta | |||
| private fun Long.isNotaFluctuation() = abs(this) > 5L | |||
There was a problem hiding this comment.
If we're hiding the unimportant/no-action-required changes by default, do we still need to filter out these fluctuations?
| append("|") | ||
| append("%.2f".format(metric.value.percentage)) | ||
| append("%") | ||
| if (metric.value.requiresAttention()) append("⚠️") | ||
| appendLine("|") |
There was a problem hiding this comment.
Question: Is the
There was a problem hiding this comment.
Yeah, I think it looks good but I also see how it can be an eye sore. I'm open to getting rid of it
There was a problem hiding this comment.
I think it's just redundant information, I support removing it
|
|
||
| analysis.metrics | ||
| .toList() | ||
| .sortedBy { it.second.percentage }.toMap() |
There was a problem hiding this comment.
Nit: This sorts in ascending order but I think we probably want descending order.
Style: When using fluent call patterns, put each chained property access and method call on its own line.
analysis
.metrics
.toList()
.sortedByDescending { it.second.percentage }
.toMap()
.forEach { metric ->
…
Issue #, if available:
N/A
Description of changes:
Readability improvements to the artifact size metrics comment/table(s)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.