Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lameventanas
Copy link

No description provided.

else {
MIME::Lite->new(%mail)->send;
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all seems like a very complicated way of writing this:

MIME::Lite->new(%mail)->send( @{ self->{send_args} || [] } )

There's no reason to care about the number of args since this code doesn't actually do anything with them. It just passes them along.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
In my testing I was having trouble with the different ways to call MIME::Lite->new, and left that code there.
I will create a new pull request without this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to create a new PR. Just force push to your fork and this PR will be updated.

@autarch
Copy link
Member

autarch commented Feb 21, 2016

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
Copy link
Member

autarch commented Feb 21, 2016

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

@lameventanas
Copy link
Author

@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
Copy link
Member

autarch commented Mar 26, 2016

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
Copy link
Author

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

@autarch
Copy link
Member

autarch commented Mar 30, 2016

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
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants