-
Notifications
You must be signed in to change notification settings - Fork 14
Truncate very long message #4
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
=======================================
Coverage 100% 100%
Complexity 25 25
=======================================
Files 1 1
Lines 115 116 +1
=======================================
+ Hits 115 116 +1
Continue to review full report at Codecov.
|
sergeymakinen
left a comment
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.
Thank you for spotting this!
I have reviewed this PR and left the comments.
It would be nice to add tests for this case. If you can't, I'll add tests by myself.
|
|
||
| /** | ||
| * @var int max character in message text. | ||
| */ |
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 think it's better to call this substitutionMaxLength because it only limits a single substitution max value length.
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.
@since 2.1 should be added.
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 your feedback.. i will change that variable name shortly
src/Target.php
Outdated
| $value = '`' . $value . '`'; | ||
| } else { | ||
| $value = "```text\n" . $value . "\n```"; | ||
| $value = "```text\n" . StringHelper::truncate($value, $this->messageMaxLength) . "\n```"; |
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.
It'd be safer to truncate a value anyway, whether it's declared as short or not.
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.
sure.. i'll revise this, then
|
Actually i don't have many experience with TDD yet. |
Hi, could you review this PR to help solving issue #3?
Thanks again for your help.