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 mail attachments 191204 #502

Closed

Conversation

Atastor
Copy link

@Atastor Atastor commented Dec 4, 2019

I am not quite sure anymore, why I didn't submit a pull request in February ...

It is useful for me and works in my app, and judging from the small number of changes, there should not be too much danger in merging this.

Regards
Michael

@wndxlori
Copy link

wndxlori commented Dec 4, 2019

This looks great, Michael, thanks. I would like to see the spec for mail updated to include a test for attachments as well?

And I'll see about getting the travis.yml file updated for the new-and-improved build process so we can actually depend on the build checks again.

@Atastor
Copy link
Author

Atastor commented Dec 5, 2019

This looks great, Michael, thanks. I would like to see the spec for mail updated to include a test for attachments as well?

Ok :-)

@Atastor
Copy link
Author

Atastor commented Dec 17, 2019

Hmm, the spec seems to run, but having to view the logs via the console muddys the water a bit... Also apparently a bundle update slipped into the pull request.

Should I delete this and start a new one?

@wndxlori
Copy link

Although I'm generally not a fan of force-push, if you want to 1) remove that last commit 2) redo without the bundle update, I'd be fine with that. This exact problem, btw, is why I almost always code-review my own commits. So I don't accidentally commit something I didn't intend. Which I have totally done in the past.

@Atastor
Copy link
Author

Atastor commented Dec 18, 2019

Ok, I promise to be better in future.
I close this pr and open a new one (#503) with a "better" set of changes.

@Atastor Atastor closed this Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants