-
Notifications
You must be signed in to change notification settings - Fork 27
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
Make it work with Python 2 #14
Conversation
coverage | ||
sphinx | ||
pytest-cov | ||
-e . |
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.
Hi, what on earth is -e . ????
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.
Forget it... I see now. :-)
This is wonderfully awesome. Many thanks for your efforts with this! Please can you address / push back / complain about my comments about the code and revise..? I'll merge this evening when I see 100% coverage with green tests across the board. I'll also test the resulting hex file on an actual micro:bit. :-) |
if version_info.major == 2: | ||
install_requires.append([ | ||
'mock', | ||
'future', |
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 don't think I actually ended up using any of the packages that future
provides (I was originally using it to provide a consistently named builtins
module, but now doing a conditional import instead), so I'll probably remove this. Just putting this note here so I don't forget :)
ae51d97
to
d46ae6c
Compare
Fixes issue ntoll#4
mock.patch('builtins.print') as mock_print, \ | ||
mock.patch('builtins.open', mock_o) as mock_open: | ||
mock.patch.object(builtins, 'print') as mock_print, \ | ||
mock.patch.object(builtins, 'open', mock_o) as mock_open: |
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.
So the reason these are now all switched to mock.patch.object() is because it supports the 2-part form where the first argument is an object not a string.
Needed because Python 2 doesn't have a builtins
module, but we're importing __builtin__
as builtins
.
Yay... great stuff and MANY THANKS for your contribution! I've tested it with a micro:bit and both Python 2.7 and 3.3+ work as advertised. |
Excellent! It was good fun to work on too :) On 22 January 2016 at 10:03, Nicholas Tollervey notifications@github.com
Matt Wheeler |
Fixes issue #4