Change check_output to use universal newlines to fix LF != CRLF

[schodge]

This should fix windows vs. *nix compatibility problems. Partially fixes Issue #57.

[schodge]

I don't understand - the Python 3 versions are failing on the .decode("utf-8") bit, but that was in the original file - the only difference is I'm now passing universal_newlines=True to check output, which shouldn't make a difference for str doesn't have attribute .decode ... ?

[hayd]

Sorry I'd missed this PR, looks like a good solution.

Apparently this returns a string (unicode) rather than a bytestring when you pass universal_newlines. Python 3 doesn't let you decode a unicode string.

If universal_newlines=True is passed, the return value will be a string rather than bytes.

This completely sucks. Not sure on best/non-hacky workaround.

[schodge]

Why is there a decode on that particular check_output but none of the others?

[hayd]

I think that was a mistake, and it should be there! Shocked it passes without it.

[schodge]

Well, since we the decode needs to be added, you assign the initial check_output to a temp variable, and do a try-decode-finally-strip() to handle it - should probably move it to it a function since it'll appear in 3 or 4 spots..?

[schodge]

OK, this branch is now a mess again, as my username didn't go through, tried to fix it ... sometime later, mess.

Anyway, I've moved all the universal newline code out of the test module and into the main file where it belongs. I then tried to write a decorator for check_output that would take it's output through a try/finally block to decode and strip, and then tried to monkey-patch check_output with the decorator. While I was at it I set the universal_newlines=True in it.

Currently, the tests are failing because my check_output is apparently None. I'm not seeing where my bug is. Attempts to fix resulted in infinite recursion, apparently the monkey-patched co calling the original calling the monkey-patched...? Anyway, at this point, I need another set of eyes on it.

Also, PEP8RADIUSPEP1 - substitute "monkey-patch" for "duck-punch" - tis cruel to punch ducks. Better to shoot them:

[hayd]

This looks like a really a neat solution...

Oh yeah, I wrote up some slides for a git workshop I gave the other day: http://slides.com/hayd/git#/

I think you haven't screwed up git (though reverting a revert is the same as just deleting the revert commit) too bad. Why not try squashing these into one commit (looks like your usename is fixed)?

"Duck punch" is cos I shamelessly stole implementation/comments from Stack Overflow!

[schodge]

If you can tell me where the bug is in my layers of wrappers causing the None and recursion, I'll fix it and squash the commits then. Just fixed part of the problem, had two git installations on this machine. Otherwise I'll try looking at this later today with fresh eyes.

[hayd]

Ah, yeah @check_output_clean has to return the function, atm it's not returning anything.

[schodge]

OK, newest commit should have everything working (I think), and I think I've cleaned up the commit log. (Emphasis on think).

[hayd]

Tests still failing on travis, looks like it's not capturing anything. Surprising (again) this passes on Windows but not Linux, seems so harmless.

You may want to attempt a git squash so all the commits are yours/not uknown see http://slides.com/hayd/git/#/11/2.

[schodge]

Irritating. Will try to look at it tonight. Will reclean the commit again then too.

Andy Hayden wrote:

Tests still failing on travis, looks like it's not capturing anything. Surprising (again) this passes on Windows but not Linux, seems so harmless.

You may want to attempt a git squash so all the commits are yours/not uknown see http://slides.com/hayd/git/#/11/2.

— Reply to this email directly or view it on GitHub https://github.com/hayd/pep8radius/pull/59#issuecomment-46504426.

[schodge]

Cleaned the commit, and looked at the Travis logs. All the errors I see are related to Hg, but check_output is used for git as well, so perhaps the problem lies with Hg? I've never been able to get the Hg tests to pass on Windows. This particular machine doesn't have Hg, but even my machines with Hg it never worked.

I'm most curious as to what this is: test_bad_rev (test_pep8radius.TestRadiusNoVCS) ... fatal: Not a valid object name random_junk_sha

λ nosetests --verbose .\tests\test_pep8radius.py
test_one_line (test_pep8radius.TestRadiusGit) ... ok
test_one_line_from_subdirectory (test_pep8radius.TestRadiusGit) ... ok
test_with_docformatter (test_pep8radius.TestRadiusGit) ... ok
test_one_line (test_pep8radius.TestRadiusHg) ... SKIP: hg not available
test_one_line_from_subdirectory (test_pep8radius.TestRadiusHg) ... SKIP: hg not available
test_with_docformatter (test_pep8radius.TestRadiusHg) ... SKIP: hg not available
test_autopep8_args (test_pep8radius.TestRadiusNoVCS) ... ok
test_bad_rev (test_pep8radius.TestRadiusNoVCS) ... fatal: Not a valid object name random_junk_sha
ok
test_bad_vc (test_pep8radius.TestRadiusNoVCS) ... ok
test_list_fixes (test_pep8radius.TestRadiusNoVCS) ... ok
test_no_vc (test_pep8radius.TestRadiusNoVCS) ... ok
test_unknown_vc (test_pep8radius.TestRadiusNoVCS) ... ok
test_using_vc (test_pep8radius.TestRadiusNoVCS) ... ok
test_version_number (test_pep8radius.TestRadiusNoVCS) ... ok

----------------------------------------------------------------------
Ran 14 tests in 3.572s

OK (SKIP=3)

[schodge]

OK, I think I've found the Hg problem... ran nosetests pdb in one terminal and poked around in the created temp directory when it halted. default is not an acceptable parameter to -r, at least on windows. After some experimenting, it looks like you want -r 0. See below.

Unfortunately, as I read the code, I don't see how to modify this for Hg. The mixin test calls TestRadius.check() directly, so I'm not sure where I'd be able to change u'default' to 0?

c:\github\pep8radius\tests\temp (master)
λ hg diff temp.py
diff -r 000000000000 temp.py
--- /dev/null   Thu Jan 01 00:00:00 1970 +0000
+++ b/temp.py   Mon Jun 30 13:05:17 2014 -0700
@@ -0,0 +1,11 @@
+def poor_indenting():
+  a = 1
+  b = 2
+  return a + b
+
+foo = 1; bar = 2; print(foo * bar)
+a=1; b=42; c=3
+d=7
+
+def f(x = 1, y = 2):
+    return x + y

c:\github\pep8radius\tests\temp (master)
λ hg diff -r 0
diff -r 000000000000 temp.py
--- /dev/null   Thu Jan 01 00:00:00 1970 +0000
+++ b/temp.py   Mon Jun 30 13:05:34 2014 -0700
@@ -0,0 +1,11 @@
+def poor_indenting():
+  a = 1
+  b = 2
+  return a + b
+
+foo = 1; bar = 2; print(foo * bar)
+a=1; b=42; c=3
+d=7
+
+def f(x = 1, y = 2):
+    return x + y

c:\github\pep8radius\tests\temp (master)
λ hg diff --stat -r 0
 temp.py |  11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

[hayd]

Had a play just now with a refactor (in what is becoming a mammoth bzr branch - bad practice). I also see the same crap with Hg output being empty. As well as a testing bug this looks like a proper bug in Hg (at least) somewhere... darn. Wait a minute, this is really confusing, it was passing and working (?) before this refactor...

Will try and fix... (as it makes for a really nice refactor).

There's still one super annoying bug that I keep seeing where comments are re-indented but I keep not getting a test case to pass up to autopep8... hmmm maybe it's # comments instead of a docstring.

[schodge]

The monkey patching may be too clever, but if you don't do it you have to remember to use the new function and not check_output() everywhere - could be an easy failure point later with program additions.

I'm not really familiar with Hg - are you sure the branch names work the same on it? I'm under the impression that there's simply no default branch being created.

I wonder if overall there's a better way to test than trying to check for identical output of a command line program that could change outside of our control. Maybe instead of checking for the expected output we cheat and use the summary stats, e.g.:

λ hg diff --stat -r 0
 temp.py |  11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

and regex out the changed/insertions/deletions?

[hayd]

not super familiar, but rev seems be worked out ok, and doing the diff manually seems to work for me, I expect it's something very silly I've done.

Need more (some) unit testing on diffs and stuff, I'm always worried it's something like that flaking out

[schodge]

I was trying to fix a unit test on another program, and came across this:

http://stackoverflow.com/questions/7562775/deriving-a-class-from-testcase-throws-two-errors

Why do we use init instead of SetUp()?

[hayd]

IIRC setUp and tearDown are done on each test, whereas __init__ is called just once - which is what we want to delete/create/commit to repo (it's expensive stuff as it is!).

[schodge]

OK, tried untangling this a bit more. IMHO, it's a tad hard to follow - the stack trace is bouncing around everywhere. (Some of which I've contributed to). The basic problem is hg diff --stat -r default is being called, instead of hg diff --stat -r 0 or hg diff --stat I'm not sure if the tests create multiple revisions in the process, but the latter option (no revision specified) should work there, as "when no revisions are specified, the working directory files are compared to its parent."

The problem appears to come from the __init__ of Radius (currently line 223, not sure how you permanently link there w/o including the whole file) in the main file, which is self.rev = self._branch_point(rev), which takes you line 405 et seq.:

    def _branch_point(self, rev=None):
        current = self.current_branch()
        if rev is None:
            return current
        else:
            return self.merge_base(rev, current)

Which, for hg, goes to line 505:

return check_output(["hg", "id", "-b"])

If you run nosetests -pdb, and hop over to the tests directory when it fails and run this you get:

c:\github\pep8radius\tests\temp (master)
λ hg id -b
default

Which means you wind up passing ['hg', 'diff', '--stat', '-r', u'default'] to check_output, which gives:

c:\github\pep8radius\tests\temp (master)
λ hg diff --stat -r default
abort: unknown revision 'default'!

So whatever problems there may be, hg is definitely getting the wrong revision number.

[schodge]

This PR has really become about two problems.

(1) check_output works differently on Python 2 & 3 because of unicode. I've currently got a monkey-patched check_output to fix the problem, which has the advantage of leaving the rest of the code intact. If we don't want to do this, we'll need to define a new function (check_output_compat()?) with the cleaning and then replace all instances of check_output() with it. And then hope we don't forget in the future and use check_output. I wonder if Travis can be setup to flag use of plain c_o.

(2) Hg command syntax. I have no idea why the syntax as-is would pass tests, but all my experimenting indicates we need to change the args used with hg to the ones in this PR.

EDIT - I'd break this into two PRs, but fixing (1) by itself will still lead to broken tests. I don't know if you'll want to merge in code that won't pass on the theory that it's less broken than existing code, or wait to merge passing code even though it addresses multiple issues. It appears our attempts are orthogonality are foiled here. :)

[hayd]

pls give the latest master a try, I've used universal_newlines in new functions shell_out. Fingers crossed Hg will pass too, like I said I think that was a one char change!