hayd / pep8radius

PEP8 clean only the parts of the files touched since the last commit, a previous commit or (the merge-base of) a branch.
MIT License
183 stars 9 forks source link

Partial windows fix #48

Closed schodge closed 10 years ago

schodge commented 10 years ago

Doesn't fix everything, but at least it gets the paths to match in terms of \ and /, so a step in the right direction.

FAIL: test_one_line (test_pep8radius.TestRadiusGit)
Traceback (most recent call last):
File "c:\Users\HodgeS\Desktop\pep8radius-master\tests\test_pep8radius.py", line 230, in test_one_line
self.check(original, modified, expected, 'test_one_line')
File "c:\Users\HodgeS\Desktop\pep8radius-master\tests\test_pep8radius.py", line 195, in check
self.assertEqual(exp_diff, out.getvalue()[:-1])
AssertionError: '--- c:/Users/HodgeS/Desktop/pep8radius-master/tests/temp\temp.py/original\n+++ c:/Users/HodgeS/Desktop/pep8radius-master/tests/temp\temp.py/fixed\n@@ -4,7 +4,9 @@\n return a + b\n \n foo = 1; bar = 2; print(foo * bar)\n-a=1; b=42; c=3\n+a = 1\n+b = 42\n+c = 3\n d=7\n \n def f(x = 1, y = 2):\n' != u'--- c:/Users/HodgeS/Desktop/pep8radius-master/tests/temp\temp.py/original\n+++ c:/Users/HodgeS/Desktop/pep8radius-master/tests/temp\temp.py/fixed\n@@ -4,7 +4,8 @@\n return a + b\n \n foo = 1; bar
= 2; print(foo * bar)\n-a=1; b=42; c=3\n+a = 1\n+b=42; c=3\n d=7\n \n def f(x = 1, y = 2):\n'
schodge commented 10 years ago

Sorry, somehow this grabbed two commits instead of one. They're the same thing, just merged branches in my fork.

hayd commented 10 years ago

We shouldn't have to use replace like this... I thought the whole point of the os.path module is that these are os independent! :cry:

actually, which bit are you fixing here, is it the diff message? If so, maybe the issue is that we are not using os.path.join in the diff output? Perhaps it's my lazy string adding that gives these weird results??

Since we're not getting "file missing" type errors I think os.path stuff is working correctly in the main code?? :s

ps could you confirm the autopep8 version you're running? (looks like this error could be that.)

schodge commented 10 years ago

Autopep8 is 1.0, there were multiple errors, I just grabbed one and pasted.

os.path isn't a problem for functionality to my knowledge (and I use it all the time, though It end to convert paths to nix anyway for simplicity) , the problem looks like the (git?) output spits out the path in nix format, and so a string comparison will return False even if they're functionally equivalent.

Actually, digging through the main code, I do think it's 391-394 that's causing the problem, since the / gets hardcoded in. Let me try changing it to us path joins and see where that gets us.

hayd commented 10 years ago

Latest is autopep8 1.0.2, updates for line ranges were in 1.0.1. :)

schodge commented 10 years ago

Strike that, lines 391-394 aren't the problem. I'm not following all of the code 100%, but I think the output that's not matching is coming from autopep8, not this script?

hayd commented 10 years ago

No, I think you were spot on, the error was coming from those lines:

https://github.com/hayd/pep8radius/commit/c8d0e7f576c4ba9e932f2785ab0a2d65c4d33175