Closed newswim closed 7 years ago
I think the way to ensure the last 2 tests pass is to rewrite the compile_template function.
The last 2 tests currently is breaking because the filepath
variable is being evaluated when the process.env.TEMPLATE_DIRECTORY
is undefined and erroring.
This didn't used to be a problem because before we wouldn't trip into the if statement and we wouldn't evaluate it.
But as we are now evaluating this variable (which will sometimes break things), we need to rethink the function
Prior to refactoring to comply with goodparts, there were some vars in odd places, and goodparts wants them always at the top of whatever scope they're in. You're right, and also moving them may have caused some of the evaluation order breakage.
On master changing the compile_templates
function should comply with goodparts and doesn't break the tests:
function compile_template(template_name, type) {
var filepathWithoutExt = process.env.TEMPLATE_DIRECTORY + '/' + template_name;
var filepath = path.resolve(filepathWithoutExt + '.' + type);
var template = !COMPILED_TEMPLATES[template_name+'.'+type]
? fs.readFileSync(filepath, 'utf8')
: '';
var compiled = Handlebars.compile(template);
// check if the template has already been opened
if(!COMPILED_TEMPLATES[template_name+'.'+type]) {
COMPILED_TEMPLATES[template_name+'.'+type] = compiled;
}
return COMPILED_TEMPLATES[template_name+'.'+type];
}
But it's a little messy
@@ Coverage Diff @@
## master #70 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 29 31 +2
=====================================
+ Hits 29 31 +2
Impacted Files | Coverage Δ | |
---|---|---|
lib/index.js | 100% <100%> (ø) |
:white_check_mark: |
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...b900ce6. Read the comment docs.
I had to disable a couple of eslint rules, namely no-console before the error check of ses.sendEmail
, and vars-on-top to handle the immediate reassignment of filepath
.
We could probably get rid of the console exception by throwing a new Error
object instead. I'm not sure how to handle that reassignment.
I think logging the ses error is fine and using eslint-disable no-console
here is ok.
I've rewritten the compile_templates
function addressing the vars-on-top
rule.
@newswim let me know what you think
Looks good! It's also a bit more readable.
@newswim Do you think we should also remove the jshint
dependecy (and it's baggage) and change the pre-commit hook to use goodparts
instead of jshint
?
Good call! I'll remove it
Looks ready to me. @nelsonic Could you have a quick look and merge if your happy.
You guys 😍 Thank you for all the work that's gone into this!
@newswim the latest version on NPM sendemail@3.1.0
contains your changes! thanks again! ❤️
Hello! This new PR is meant to add GoodParts compliance a la Issue #66 and re-initialize the merge from within the upstream repo rather than my own fork.
It has the following TODOs:
sendemail
is not returning anything, so tests 10 and 11 never end (see failure in action).