search

houseabsolute / Log-Dispatch

Dispatches messages to one or more outputs
https://metacpan.org/release/Log-Dispatch/
Other
12 stars 29 forks source link

Add send_args to Log::Dispatch::Email and subclasses #17

Open lameventanas opened 8 years ago

autarch commented 8 years ago

I like the idea but this has some areas that need fixing. Also, I wonder if this should be named mailer_args to better match Log::Dispatch::Email::EmailSend, which already accepts an argument of that name.

autarch commented 8 years ago

BTW, thanks for the patch! I should've said that in the first place.

lameventanas commented 8 years ago

@autarch I created a repository for Log::Dispatch::Email::EmailSend based on 0.03 (I couldn't find it in your github account): https://github.com/alan-ml/Log-Dispatch-Email-EmailSend

I made a backward-compatible change so that it accepts send_args as well as mailer_args.

PS: in my tests, mailer_args was not even used before my changes.

autarch commented 8 years ago

I made a new repo for EmailSend at https://github.com/houseabsolute/Log-Dispatch-Email-EmailSend

That said, I'm not sure it's worth fixing this module. Email::Send is deprecated in favor of Email::Sender.

lameventanas commented 8 years ago

@autarch can you take a look at the send_args-test.pl script? I invested a lot of time in these tests.

autarch commented 8 years ago

Hi @alan-ml, thanks for working on this. I'm not sure what to do with this script though. I don't run exim on my systems, so I can't really use it, and I'm really more interested in tests that can be automated anyway. Testing email sending with these legacy modules is really challenging since they don't provide a way to mock the actual sending.

That said, I already made some earlier comments on this PR that you haven't addressed. If you could take a look and respond/fix the issues, that'd make it easier for me to merge this. I probably won't merge the test script though, since I don't think I'd ever run it. I suppose it could go in eg/ or something like that.

lameventanas commented 8 years ago

Yes, the test script is not an automatic test (would love that, but its not possible). Its more of a guide on how to pass arguments to each mail module. Each one has different ways, and own caveats. The comments reflect each test, what worked and what didn't.

I'm also thinking about adding this information in each module's POD section. It would be more visible, but it could also look scary for someone looking at this for the first time. What do you think?