-
Notifications
You must be signed in to change notification settings - Fork 31
cleaning up filesystem errors on build-static #421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
mkdirp('uw-frame-static/target/css/themes/', function (err) { | ||
if (err) throw error; | ||
mkdirp('uw-frame-static/target/css/themes/', function (error) { | ||
if (error) throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could error
be confused with Error
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to trust that Error
would be immediately noticeable due to classes starting with capitals by convention. If someone saw Error
and not Error()
or new Error()
that should be picked up during review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, you are expecting Google code style's new-cap
lint rule to catch this.
tools/uw-frame-static/build.js
Outdated
@@ -12,9 +12,12 @@ var exec_handler = function(error, stdout, stderr) { | |||
|
|||
var mkdirp = require('mkdirp'); | |||
var copy = require('recursive-copy'); | |||
var copyOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming Node.js 4+ is the target run time, should this be a const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. and we're using const elsewhere in the file. I'll update the file to use it consistently.
changes to frame weren't showing up when I ran static:dev, so I went through and fixed the build script.
Standardized error throwing, copy now overwrites.
Contributor License Agreement adherence: