-
Notifications
You must be signed in to change notification settings - Fork 64
Conversation
@seemethere Looks good! One major thing, though—it'd be nice to have some tests for this. Can you add a test to |
@evhub I was just thinking about that too! |
Changes Unknown when pulling 2df4c11 on seemethere:dry_run into * on Yelp:master*. |
@evhub Tests have been added! |
@@ -6,6 +6,7 @@ | |||
import os.path | |||
|
|||
import mock | |||
from six import StringIO |
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'd rather not add a new dependency just to get StringIO
. Can you do
try:
from io import StringIO
except ImportError:
from StringIO import StringIO
instead?
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.
Isn't that exactly what six
does?
I agree with leaning against adding dependencies, but six
seems to be a pretty standard one, especially for ensuring 2/3 compat
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, fair point—I retract my request to remove six
. If we're using six
here, though, we should probably use it elsewhere also—but I can take care of that after this is merged.
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.
imo use six, it's what it's for
But in this case you can avoid it entirely, I'll comment below :D
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.
and actually, this is just a test so it shouldn't have been added as a dependency in setup.py (setup.py is for runtime dependencies, whereas this is a test-time dependency (and therefore belongs in requirements-dev.txt))
LGTM! @asottile Do you want to weigh in here before I merge this? |
@@ -62,6 +63,13 @@ def test_directory(): | |||
assert _read_input_file() == method_to_function_output_contents == _read_output_file() | |||
|
|||
|
|||
def test_dry_run(): |
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.
you want the capsys
pytest fixture: http://doc.pytest.org/en/latest/capture.html#accessing-captured-output-from-a-test-function
eheh, didn't meant to get terribly involved in the project but sure :D |
I haven't used undebt yet, but one thing that would make this specific thing more useful to me is a diff-mode output. As it stands, I'd have to either not use the dry run option and use git for my diff, or I use dry run, and pipe it to a file to |
@Daenyth that's a great idea. Most of these fixer type tools (especially the ones we use with pre-commit like my import reorderer, yapf, autopep8, etc.) have prior art. Want to open an additional issue? |
For dry-run to have a "show diff" variant? Sure |
@evhub Removed six dependency, switch to capsys |
Changes Unknown when pulling 04ebc77 on seemethere:dry_run into * on Yelp:master*. |
2 similar comments
Changes Unknown when pulling 04ebc77 on seemethere:dry_run into * on Yelp:master*. |
Changes Unknown when pulling 04ebc77 on seemethere:dry_run into * on Yelp:master*. |
def test_dry_run(capsys): | ||
args = ["undebt", "-i", method_to_function_input_path, "-p", method_to_function_path, "--verbose", "--dry-run"] | ||
with mock.patch("sys.argv", args): | ||
main() |
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.
We us this pattern in a lot of other repositories (could make this so you don't need monkeypatching):
def main(argv=None):
parser = argparse.ArgumentParser()
...
parser.pase_args(argv)
By default, argparse will go to sys.argv if the passed in argument is None
otherwise it'll use the list of arguments.
This lets you call main(('-i', ..., ..., ...))
under test
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'm going to merge this and then try to address all of the issues brought up in this thread. |
Adds a
--dry-run
flag that outputs all output to stdout.Example usage:
Example output:
Related to issue #3