Truncate on bytes or chars #88
Conversation
| @@ -62,18 +63,30 @@ public String toString() { | |||
| } | |||
|
|
|||
| /** | |||
| * Builds the string such that it does not exceed maxSize, including the truncation string. | |||
| * Builds the string such that it does not exceed maxSize in bytes, including the truncation string. | |||
| * | |||
| * @param maxSize the maximum size of the resultant string. | |||
| * @return A string comprising as many of the joined chunks that will fit in the given size, plus the truncation | |||
| * string if truncation was necessary. | |||
| */ | |||
| public String build(final int maxSize) { | |||
There was a problem hiding this comment.
Should we deprecate this one?
There was a problem hiding this comment.
yes, I think we should.
And instead of passing the chunkOnChars boolean field, would it be better if we expose two methods directly?
There was a problem hiding this comment.
Sure; preference for names? buildByBytes/buildByChars?
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
============================================
- Coverage 87.11% 85.90% -1.21%
- Complexity 142 150 +8
============================================
Files 16 17 +1
Lines 652 731 +79
Branches 49 54 +5
============================================
+ Hits 568 628 +60
- Misses 66 85 +19
Partials 18 18
Continue to review full report at Codecov.
|
| builder.addText("🕴️🕴️\n"); // 2 + 1 | ||
| assertThat(builder.build().toString().length()).isEqualTo(11); | ||
| assertThat(builder.build().toString().getBytes(StandardCharsets.UTF_8).length).isEqualTo(25); | ||
| assertThat(builder.build().build(20, chunkOnChars)).isEqualTo(chunkOnChars ? "☃☃☃\n🕴️🕴️\n" : "☃☃☃\nTruncated"); |
There was a problem hiding this comment.
I don't think we should mix the tests. This test case is named "shouldTruncateToBytesNotChars" but you are still testing if it truncates by chars correctly, that's a little redundant and confusing. Same for others, maybe split them.
There was a problem hiding this comment.
Ah yes, the test name was from before I decided to make it switchable. I will definitely update that.
My argument for having the parametrized like this is better combinatorial coverage, but it also helps highlight the difference in behaviours - ie, given a certain input, the output will look like this or that depending on the chunking mode.
If you'd rather I split it out I can do, but my preference would be to leave it parameterized.
There was a problem hiding this comment.
but at least we should change the name? maybe shouldTruncateOnBothBytesAndChars？
| @@ -62,18 +63,30 @@ public String toString() { | |||
| } | |||
|
|
|||
| /** | |||
| * Builds the string such that it does not exceed maxSize, including the truncation string. | |||
| * Builds the string such that it does not exceed maxSize in bytes, including the truncation string. | |||
| * | |||
| * @param maxSize the maximum size of the resultant string. | |||
| * @return A string comprising as many of the joined chunks that will fit in the given size, plus the truncation | |||
| * string if truncation was necessary. | |||
| */ | |||
| public String build(final int maxSize) { | |||
There was a problem hiding this comment.
yes, I think we should.
And instead of passing the chunkOnChars boolean field, would it be better if we expose two methods directly?
|
This seems a bit incomplete in that TruncatedString now supports counting bytes and chars, but ChecksOutput always tells it to count bytes and does not let checks publisher plugins access the TruncatedString themselves. I'm not filing an issue about that because I don't know whether any such plugin would even need to count chars:
|
No description provided.