Update bl to v4.0.0, rename a few vars, use template literals.#1
Update bl to v4.0.0, rename a few vars, use template literals.#1XhmikosR wants to merge 0 commit intorvagg:masterfrom XhmikosR:master
Conversation
|
BTW I also tried using the cross-spawn package here and it also failed with the single quotes, which I'd expect it to escape. But maybe I missed something. I'll wait until @rvagg you can have a look :) To see this in action see https://github.com/XhmikosR/gitexec/actions |
|
You should try to keep the scope of individual commits relatively isolated, it's really hard to review and tbh I'd even prefer to deal with these things as separate PRs. I don't really mind what you've done here, it's just hard to extract the important pieces. From what I can tell the essence of what you're proposing this: const child = spawn('bash', [ '-c', gitcmd ], { env: process.env, cwd: repoPath })
const child = spawn(gitcmd, { env: process.env, cwd: repoPath, shell: true })So just switching from a Would you mind trying to pull this into 3 separate commits? The |
|
NP, this was mostly opened as a preview. I'll split the patches now. |
|
@rvagg done. This PR should be merged last and might need a rebase. |
|
Found a weird bug in GitHub's push-to-fork functionality, I pushed to your master and it closed this PR, believing that there was no diff to my master I suspect. Anyway, managed to work my way out of it and this is now on master and landed. Thanks for fixing the single-line |
The thing is that this breaks at least on Windows if you use single quotes for the command. So, I'm not sure if this will cause any breakage upstream.
And being that we don't have CI yet on https://github.com/nodejs/changelog-maker I can't tell for sure...
PS. You need to register for GitHub Actions CI.
PPS. I had to use 8.16.1 because there's a bug in actions/setup-node and installs Node.js 8.10 which lacks
npm ci