Goodparts refactor -- resolves #66

[newswim]

Just one hanging point of review for me. On line 57, i wasn't totally sure about the {returns} value for JSDoc. Next time before tackling something like this, I should read over the source code (and readme) fully. If someone doesn't mind double checking me 🏁 --- that would make you the best 🏆

[newswim]

Current issue with Travis comes from that same error, TEMPLATE_DIRECTORY not being set. I resolved that locally, which leads to an error in one of the tests. Pursuing fix...

[nelsonic]

@shouston3 would you mind peer-reviewing this PR? (please/thanks!) (if you can help @newswim make the tests pass it would be ace!)

[samhstn]

@newswim You are now throwing an Error object here instead of a string.

So you should look to handle an object rather than a string in the tests here and in the other try catch statements

Instead of looking at e.indexOf, look at e.message

[newswim]

Did you want tests resolved here or in a different pr? This is only meant to address #66

On Feb 7, 2017 2:56 AM, "Sam Houston" notifications@github.com wrote:

@newswim https://github.com/newswim You are now throwing an Error object here https://github.com/dwyl/sendemail/pull/69/files#diff-6d186b954a58d5bb740f73d84fe39073R19 instead of a string.

So you should look to handle an object rather than a string in the tests here https://github.com/dwyl/sendemail/blob/master/test/index.test.js#L22 and in all of the other try catch statments

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dwyl/sendemail/pull/69#issuecomment-277938238, or mute the thread https://github.com/notifications/unsubscribe-auth/AGW7WFExTl5lLPPDs6QJ8vMKB0irqY5jks5raDHRgaJpZM4L5J6g .

[samhstn]

The only way to address the failing tests and throw an error object is to change how we're handling the error in the tests

So I'd say change the tests just in regards to reading e.message instead of e.indexOf and leave the rest of the test file.

[newswim]

Ah, reread your message, Sam. I'll resolve this tomorrow. Thank you for looking!

[newswim]

Almost there, I'm not sure about these last two tests. I'll go ahead and push what I've done. Is it normal with tape to get a error Command failed with exit code 1. after all the tests run?

[samhstn]

No it should be exit code 0

[newswim]

I just realized I'm not testing the type param. This seemed to be causing the tests to fail, whereas hardcoding them passed:

=============================== Coverage summary ===============================
Statements   : 96.67% ( 29/30 )
Branches     : 90% ( 9/10 )
Functions    : 100% ( 4/4 )
Lines        : 96.67% ( 29/30 )
================================================================================

[samhstn]

Note the compile_template function needs two arguments. You are currently passing one and that could be causing the breakage

[samhstn]

Also, I think the email function should have the template name as the first argument and I think you've passed in a filename

[newswim]

hey @shouston3, thanks for looking at this.

I had changed that because I was getting this error:

Error: ENOENT: no such file or directory, open './examples/templates/hello'

This is probably going to take some reworking.

[newswim]

Got it, filepath was getting declared wrong.

[newswim]

🆘

I'm not sure how to test this, and I have a dentist appt in 15 minutes. I'm not sure how to handle this returned callback, please halp 😬

[newswim]

Looks like an env variable is still not getting set. Sorry to have to leave it like this, be back in a few hours.

[newswim]

Well.. a little further. There was an error, and I guess I need to get aws keys now.

ses.sendEmail :: err: { InvalidClientTokenId: The security token included in the request is invalid.

[newswim]

I believe this is now a matter of the env variables within the Travis machine not getting set.

see: https://travis-ci.org/dwyl/sendemail/builds/198932105#L126 compared with: https://travis-ci.org/dwyl/sendemail/builds/199391820

[codecov-io]

Codecov Report

Merging #69 into master will not impact coverage by -3.34%.

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage     100%   96.66%   -3.34%     
==========================================
  Files           1        1              
  Lines          29       30       +1     
==========================================
  Hits           29       29              
- Misses          0        1       +1

Impacted Files	Coverage Δ
lib/index.js	96.66% <100%> (-3.34%)	:x:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 62f7ad4...9924704. Read the comment docs.

[newswim]

Hmm.. my guess is that that last test isn't passing because of the other environment variables.

I just realized it's late in the day for you guys, so I'll leave this and wait until it has time for review tomorrow. I would also really like to know how environment variables are typically set with Travis (it's my first time using it). I wonder if this has something to do with:

Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.

https://docs.travis-ci.com/user/environment-variables/#Defining-public-variables-in-.travis.yml

[samhstn]

@newswim Nice find, I think 'encrypted environment variables not available to forks' seems reasonable, since my tests were passing locally.

I think what you added to the travis.yml file makes sense

[nelsonic]

@newswim you should have access to push your branch to this repo and then open a new pull request to test that theory... 👍

[newswim]

Thanks Sam, I'll initiate a new PR from the this repo rather than my fork. Feel free to close 🔚

[samhstn]

Closing see #70 instead