search

workshopper / learnyounode

Learn You The Node.js For Much Win! An intro to Node.js via a set of self-guided workshops.
Other
7.25k stars 1.84k forks source link

FIX: Make It Modular - Allow optional encoding parameter #502

Closed robbiedhickey closed 7 years ago

robbiedhickey commented 7 years ago

This resolves https://github.com/nodeschool/discussions/issues/1999. Searching through the history I believe a few other folks have run up against this as well.

This fixes a false negative users are receiving on the Make It Modular course when calling fs.readdir with its optional encoding parameter. Any solution that passes the optional parameter will erroneously fail, because the workshopper assumes that the second parameter will always be the callback:

fs.readdir(filePath, 'utf8', function(err, files) {});

This PR also includes a test case to validate that the false negative is resolved.

martinheidegger commented 7 years ago

This PR is awesome. It closes #508 and it closes #497. Thank you for your effort!

martinheidegger commented 7 years ago

Published as 3.5.9

AnshulMalik commented 7 years ago

Hey @hodorswit , The builds were failing for node v5.0, so I used git bisect to investigate which commit was causing the build to fail, and I found this was the one.

Can you please test this once again for node v5.0. Thanks

robbiedhickey commented 7 years ago

@AnshulMalik thanks for the heads up! I'm in the middle of a move right now but will investigate as soon as possible.

robbiedhickey commented 7 years ago

@AnshulMalik The Node v5.12.0 Documentation indicates that this version does not support the optional encoding parameter.

My initial thought would be to check process.version to see if the user's current process is at least version 6, and only execute the encoding parameter test under that scenario. It's kind of hacky, but allows us to retain test coverage for later versions. Thoughts?

AnshulMalik commented 7 years ago

Sounds good, we can add it as a patch till we support node 5 :)