Closed jmaslak closed 9 years ago
Thanks. Mostly I like it. I have a couple things I'd like to see changed, though, before merging.
Sorry for the delay in getting back to you.
I think both those changes shouldn't be a big deal - I'll put something together for you to look at in the next day or two.
I have refactored the PR to eliminate the File::Slurper dependency and to make the decision logic for the comment body easier to decipher (I hope!). I also broke the comment logic into its own sub, which I think makes things more clear.
Certainly if there is anything else you would like to see changed in this PR (or if you don't think I did what you were hoping I would do), please let me know.
Thanks for the revisions. I squashed them and merged it into master.
After I see travis smokes, I'll ship a new release.
Awesome! Thanks for the feedback and merging the PR.
On Fri, Aug 21, 2015 at 3:45 PM, David Golden notifications@github.com wrote:
Thanks for the revisions. I squashed them and merged it into master.
After I see travis smokes, I'll ship a new release.
— Reply to this email directly or view it on GitHub https://github.com/cpan-testers/CPAN-Reporter/pull/30#issuecomment-133571771 .
This is my second attempt at this - I believe this one will actually pass the TravisCI build.
I implemented this in a way that keeps the smoke text "notice" but adds the contents of comment.txt to the report when doing automated testing. When doing interactive testing, it will replace the default comment text.
I'm glad to take feedback about this and modify this if you like the idea but not the implementation.
I don't think there is much controversial here except perhaps the choice to add File::Slurper to the list of pre-requisites this module depends on. If you don't like that choice, let me know what your preferred alternative module is or that I should implement a simple open, read, close routine (and similar for write).