Conversation
alangpierce
left a comment
alangpierce
left a comment
There was a problem hiding this comment.
Thanks! Some thoughts, and there's a test failure, but generally makes sense to me.
|
|
||
| async function mkdTempSafe (prefix) { | ||
| let parts = normalize(prefix).split(sep); | ||
| // let last = parts.pop(); |
There was a problem hiding this comment.
Looks like you left this line in by mistake?
| async function runWithTemplateDir(exampleName, fn) { | ||
| let suffix = Math.floor(Math.random() * 1000000000000); | ||
| let newDir = `./test/tmp-projects/${exampleName}-${suffix}`; | ||
| let newDirPref = `./test/tmp-projects/${exampleName}/tmp-`; |
There was a problem hiding this comment.
Can you use the whole word "prefix" instead of "pref"? In my mind, "pref" means "preference".
Also, looks like you added an extra level of nesting here, which is causing the test failure. I'd rather just keep the same nesting level as before (which also make makes it easier to potentially avoid mkdTempSafe), but I don't have a strong preference.
|
|
||
| let originalCwd = process.cwd(); | ||
|
|
||
| async function mkdTempSafe (prefix) { |
There was a problem hiding this comment.
Can you add a comment explaining what safe means here? Looks like it's doing the equivalent of mkdir -p? You could probably also skip this by making sure the tmp-projects folder always exists. I think one way people do that is to commit an empty .gitkeep file in the directory (since git doesn't allow empty tracked directories).
| throw e; | ||
| } | ||
| } | ||
| return await mkdtemp(prefix); |
There was a problem hiding this comment.
Thanks, I hadn't seen mkdtemp. Does this fix anything for Windows, or is it just a more robust alternative to my manual random directory thing?
| import assert from 'assert'; | ||
| import { exec } from 'mz/child_process'; | ||
| import { exists, readFile, writeFile } from 'mz/fs'; | ||
| import { exists, readFile, writeFile, mkdtemp, mkdir } from 'mz/fs'; |
There was a problem hiding this comment.
Actually, looks like mkdtemp was introduced in node 5, and I'd like to support node 4, so probably best to stick with the old random number approach unless there's a problem with it.
|
BTW, I was running into nodegit issues when setting up the build for https://github.com/decaffeinate/decaffeinate-examples . Turns out it doesn't build at all on node 7 ( nodegit/nodegit#1157 ). So that's more motivation to drop it as a dependency. |
2 participants
No description provided.