Remove stray jpegtran temp file in img task #64

Merged
merged 1 commit into from Aug 29, 2012

Projects

None yet

2 participants

@robinpyon

New to node and OSS development :) Please let me know if there's a better way to go about this / if there's a contributor's guide.

@mklabs mklabs commented on an outdated diff Aug 29, 2012
tasks/img.js
grunt.helper('which', 'jpegtran', function(err, cmdpath) {
if(err) return grunt.helper('not installed', 'jpegtran', cb);
+
+ // Remove temp JPEGtran file when finished
+ function clean() {
+ if (fs.exists(tmpfile, function(exists) {
@mklabs
mklabs Aug 29, 2012

Asynchronous function doesn't usually return value in a way that we can use it like this. The boolean is carried over by the exists value you get in the callback. Can you drop this if statement around fs.exists ?

@mklabs
mklabs Aug 29, 2012

Related, fs.exists is a new API introduced in node 0.8.x, which unfortunately would break any 0.6.x based install. Can you, at the top of the file, do something like:

// other dependencies, next to the list of requires
var exists = fs.exists || path.exists;

and then use this exists reference instead?

@mklabs mklabs commented on an outdated diff Aug 29, 2012
tasks/img.js
grunt.helper('which', 'jpegtran', function(err, cmdpath) {
if(err) return grunt.helper('not installed', 'jpegtran', cb);
+
+ // Remove temp JPEGtran file when finished
+ function clean() {
+ if (fs.exists(tmpfile, function(exists) {
+ if (exists) {
@mklabs
mklabs Aug 29, 2012

Early return pattern whenever possible, especially true for within all these callbacks. The else statement consists in just calling the callback. Can you turn it into something like:

if(!exists) return cb();

// rest of the code
@mklabs mklabs commented on an outdated diff Aug 29, 2012
tasks/img.js
grunt.helper('which', 'jpegtran', function(err, cmdpath) {
if(err) return grunt.helper('not installed', 'jpegtran', cb);
+
+ // Remove temp JPEGtran file when finished
+ function clean() {
+ if (fs.exists(tmpfile, function(exists) {
+ if (exists) {
+ grunt.log.subhead('** Removing: ' + tmpfile);
+ fs.unlink( tmpfile, function(err) {
+ if (err) {
+ grunt.log.error(err);
@mklabs
mklabs Aug 29, 2012

missing return. Plus, the grunt callback should get false to indicate the failure. return cb(false); Ideally, it should be just return cb(err); but right now we're forced to log the error and call the callback with a false value. Will change in grunt 0.4

@mklabs
Owner

Hi Robin,

Thanks for taking the time to look over the project to make it better. I added a few comments for you inline, I'll be happy to merge this once you've changed these few little things. Thanks!

@mklabs
Owner

Should close #49

@robinpyon

Thanks for your feedback and advice. I just added another commit and rebased it before pushing—not 100% sure if this is the correct procedure, let me know if I should be doing things differently.

@mklabs
Owner

Hmm, Github got confused with another Pull request merge. Discussion goes into "View outdated diff" view. It seems I can still automatically merge this though.

@mklabs
Owner

oh, we crossed messages. You did well, rebase + forced push is the right thing to do. Nice to see that GitHub handles it well.

Reviewing, but all seems super good. The very last thing that would be cool is to turn the clean function into a grunt helper, but I don't really mind. Great PR, thanks and merging.

@mklabs mklabs merged commit e55b5d0 into mklabs:master Aug 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment