Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix is_virtualenv in Windows #178

Merged
merged 3 commits into from Aug 3, 2016

Conversation

guykisel
Copy link
Contributor

@guykisel guykisel commented Jul 17, 2016

I believe this should fix #159 and #163

I also updated the is_a_venv test case so that it'll work properly in Windows.

Slightly unrelated, but you might also be interested to know that I'm using prospector in https://github.com/guykisel/inline-plz-bot so thanks for maintaining such a great tool 馃槃

@coveralls
Copy link

coveralls commented Jul 17, 2016

Coverage Status

Coverage increased (+0.07%) to 59.135% when pulling f72fe2a on guykisel:fix-windows-pathutils into d07f6d4 on landscapeio:master.

@coveralls
Copy link

coveralls commented Jul 17, 2016

Coverage Status

Coverage increased (+0.02%) to 59.093% when pulling 08d995b on guykisel:fix-windows-pathutils into d07f6d4 on landscapeio:master.

dircontents = os.listdir(path)
try:
dircontents = os.listdir(path)
except (OSError, TypeError):
Copy link
Contributor Author

@guykisel guykisel Jul 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TypeError here might seem odd, but without it I get this:

py27 runtests: commands[0] | nosetests tests
....E........................
======================================================================
ERROR: Windows doesn't allow extremely long paths. This unit test has to be
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\Guy\Documents\prospector\tests\finder\test_file_finder.py", line 50, in test_long_path_not_a_venv
    self.assertFalse(is_virtualenv(path))
  File "C:\Users\Guy\Documents\prospector\prospector\pathutils.py", line 12, in is_virtualenv
    dircontents = os.listdir(path)
TypeError: must be (buffer overflow), not str

----------------------------------------------------------------------
Ran 29 tests in 0.141s

@carlio
Copy link
Member

carlio commented Jul 17, 2016

@guykisel Thanks for this! The only machine I have running Windows had a motherboard failure a few months ago so never got around to it myself!

This seems to be related: https://bugs.python.org/issue1776160

Does this therefore work on Windows now? It seems like Python itself is not bothered, if so there's not much that can be done except catch the exception like you have done :)

@guykisel
Copy link
Contributor Author

This does work on Windows now as far as I can tell. I'm testing on Windows 10 with Python 2.7.11.

It does still seem like the original issue from #159 might still exist, where prospector is not honoring ignore-paths in Windows. My fix at least makes it a little less painful.

@guykisel
Copy link
Contributor Author

You could maybe set up https://www.appveyor.com/ to get some Windows coverage.

@guykisel
Copy link
Contributor Author

Don't merge yet, I actually have to make the path in the test longer - I forgot that my local repo's path is already a few directories deep.

@guykisel
Copy link
Contributor Author

Updated with a much longer path to guarantee that this test always goes over the Windows maximum path length.

@coveralls
Copy link

coveralls commented Jul 31, 2016

Coverage Status

Coverage decreased (-0.08%) to 58.987% when pulling b73078a on guykisel:fix-windows-pathutils into d07f6d4 on landscapeio:master.

@carlio carlio merged commit c68e152 into landscapeio:master Aug 3, 2016
@carlio
Copy link
Member

carlio commented Aug 3, 2016

@guykisel Finally got around to making a release today, thanks!

@guykisel guykisel deleted the fix-windows-pathutils branch August 3, 2016 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants