add getSignatureRegex/setSignatureRegex #45
Conversation
|
@willdurand ping? |
|
@willdurand any chance to get this merged and released? so i could stop using own fork. |
| public function testCustomSigRegex() | ||
| { | ||
| $regex = $this->parser->getSignatureRegex(); | ||
| $this->parser->setSignatureRegex($regex); |
There was a problem hiding this comment.
Does this test case actually test anything?
There was a problem hiding this comment.
seems not other than the method call doesn't fail. what you suggest to test there?
glensc
force-pushed
the
set-sig-regex
branch
from
bed67e0 to
a7da53e
Compare
|
rebased against current master! |
|
@glensc how would you feel about giving me a hand on this project (by becoming a contributor)? |
|
@willdurand sure, i can get this merged then :D |
|
perfect, but don't forget to add a test case first 😉 |
|
i was hoping to bypass that 😈 but what to test there really? |
|
I think you could add a test case similar to the other ones (with a fixture file) but involving a different signature since you obviously need that: it will document the use case you have that led to this patch. |
testcase showing that ls -l listing is treated as signature for each file
|
alright, i'll take testcase from #42 |
|
ok, looks good? i think 2.7.0 is on the way. and i think i'll create example: https://github.com/eventum/eventum/blob/master/CHANGELOG.md |
willdurand
left a comment
willdurand
left a comment
There was a problem hiding this comment.
Looks good, thanks! You can merge it if Travis-CI agrees.
| /** | ||
| * override regexp, not to match too greedy signature. | ||
| * | ||
| * @see https://github.com/willdurand/EmailReplyParser/pull/42 |
There was a problem hiding this comment.
I would simply add this:
// See: https://github.com/willdurand/EmailReplyParser/pull/42.
There was a problem hiding this comment.
@see is official @phpdoc annotation, phpstorm didn't make links clickable in the past. seems that's not the case anymore. can drop.
glensc
force-pushed
the
set-sig-regex
branch
from
05edcd3 to
7c64d7d
Compare
|
|
||
| /** | ||
| * @return string | ||
| * @since 2.7.0 |
There was a problem hiding this comment.
Ah I did not pay attention to that, usually I dont use this and I dont think we should be using this. The CHANGELOG file (or GitHub release) is way more expressive.
There was a problem hiding this comment.
ok, so remove the annotation and commit to master?
2 participants
this is mostly to provide gateway for issues like #42
also, could you tag a release after this gets merged, thanks!