-
Notifications
You must be signed in to change notification settings - Fork 16
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
updated to work in both python 2 and 3 #16
Conversation
Thanks!! |
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.
some stuff I noticed.
from blinkytape import BlinkyTape | ||
from time import sleep | ||
from xml.etree import ElementTree | ||
import urllib2 | ||
import sys | ||
if (sys.version_info > (3, 0)): |
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 don't need the outer parens here.
print("Usage: python Aurora.py -p <port name>") | ||
print("(ex.: python Aurora.py -p /dev/ttyACM0)") |
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.
doesn't that need the from future import print_function to work on py27?
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.
'from future import print_function' hasn't been needed in a while. I can't recall since when but it certainly has been for a while.
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're right, a current py27 does not need it.
maybe it was a thing with < 2.7 or early 2.7.
port = serialPorts[0] | ||
# port = serialPorts[0] | ||
port = 'com3' |
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.
accidental change?
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.
intentional as that system doesn't work on wiondows
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.
Well, now it only works on windows and only on com3, that's not an improvement I guess.
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.
also has nothing to do with PR scope.
temp_baseline = 60 | ||
|
||
temp_baseline = 55 # configure to you base line temp here |
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.
accidental change?
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.
intentional. Please note the comment. Different GPUs have different baseline temps.
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.
the title of this pull request says "updated to work in both python 2 and 3", thus changes of this kind are ... unexpected.
green = min(254, max(0,100 - color_temp)) | ||
red = min(254, max(0,0 + color_temp)) |
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.
pep8: have a space after the comma.
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 wasn't really focused on formating there are a lot of other style violations in the codebase. I might do another round sometime soon.
@@ -0,0 +1,2 @@ | |||
from .moving_dot import MovingDotMode, WideMovingDotMode | |||
from .modes import RandomFlashMode, FillUpMode, FlashMode, PoliceMode, PoliceMode2 |
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.
add a newline to make diff happy.
@@ -7,7 +7,7 @@ class RandomFlashMode(FixedColorMixin, BaseMode): | |||
last = 0 | |||
|
|||
def calc_next_step(self): | |||
self.colors[self.last] = 0, 0, 0 | |||
self.colors[self.last] = (0, 0, 0) |
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.
parens not needed.
self.colors[i] = 0, 0, 0 | ||
for i in range(self.led_count / 2, self.led_count / 2 + self.mid_width / 2): | ||
self.colors[i] = 0, 0, 254 | ||
for i in range(int(self.led_count / 2 - self.mid_width / 2), int(self.led_count / 2)): |
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.
there is also the //
integer-result division operator in py27 and py3.
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.
that would be cleaner
Thanks for the detailed reports, I should have looked through this more closely. Did you happen to make any of the changes? |
No, I did not implement any of these. I just did some py3 ports of misc. software in the past, so i thought i give some feedback when this popped up in my inbox. Also, still very happy with my blinkinlabs stripe. :-) |
Maybe I should ask this- is there a strong desire to continue supporting Python 2? I'd be fine with deprecating it since it's been sunset. |
For the library, it might make sense to support py27 and py3 in parallel, as people might have still some apps in py27 using the library. For examples, the code could be as well py3 only. Also, it is of course easier to just support python 3.5+ than supporting py27 and py3 at the same time. |
No description provided.