-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature dry run #7
Feature dry run #7
Conversation
@@ -1,60 +1,69 @@ | |||
import {exec} from "child_process"; |
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.
If you've changed this just to be able to patch this in tests, you don't need to...
Raw:
import {exec} from "child_process";
exec();
babel output:
var _child_process = require("child_process");
(0, _child_process.exec)();
The same applies after refactoring to sinon (for sandbox.stub(childProcess, "exec")
) where it would still work.
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.
Good catch. I'm not sure why I thought I needed to do that.
@jmcriffey -- First review pass done. Let's refactor to not patch exec. Feel free to hit me up on slack if you want to chat game plan more! |
import fileUtils from "./file-utils"; | ||
import fs from "fs"; | ||
import logger from "./logger"; | ||
import mockfs from "mock-fs"; |
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.
Doesn't merely importing this start the monkey-patching of node internals? I think we want a lazy require
in here right before we actually need to start doing stuff...
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.
Yeah that is correct. It doesn't actually replace anything in the file system until you call it, but I'll go ahead lazy require it to make sure we correctly read the package file.
@ryan-roemer This is ready for another look over. It has also been updated so that |
|
||
exec(cmd, cb) { | ||
if (git.dry) { | ||
exec("git status", cb); |
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 like the idea of calling git status
here in place of any destructive git commands to make sure git is callable.
Thoughts?
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.
What are all the commands we can run? git status
seems like a good dry run counterpart to git checkout
. Some other git commands have a dry run capability IIRC.
@jmcriffey -- One question, rest lgtm without re-review unless you want it. |
@ryan-roemer
Adds a dry run command that runs through the postversion and postpublish scripts using
mock-fs
. Addresses #5.Here is what the dry run command looks like run on publishr itself: