Skip to content
This repository was archived by the owner on Dec 4, 2022. It is now read-only.

Conversation

@adipriyantobpn
Copy link

Hi, could you review this PR to help solving issue #3?
Thanks again for your help.

@codecov
Copy link

codecov bot commented Aug 17, 2017

Codecov Report

Merging #4 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master     #4   +/-   ##
=======================================
  Coverage       100%   100%           
  Complexity       25     25           
=======================================
  Files             1      1           
  Lines           115    116    +1     
=======================================
+ Hits            115    116    +1
Impacted Files Coverage Δ Complexity Δ
src/Target.php 100% <100%> (ø) 25 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae9c030...a1d5389. Read the comment docs.

@sergeymakinen sergeymakinen self-assigned this Aug 17, 2017
Copy link
Owner

@sergeymakinen sergeymakinen left a 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.
*/
Copy link
Owner

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.

Copy link
Owner

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.

Copy link
Author

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```";
Copy link
Owner

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.

Copy link
Author

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

@adipriyantobpn
Copy link
Author

Actually i don't have many experience with TDD yet.
Can you help me to add the test, please?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants