Code cleanup #67

lechat · 2016-10-30T03:49:04Z

Mostly cosmetic changes to code to look closer to idiomatic python:

removed unused imports
gave one character variables meaningful names
fixed all "missing space after" or "extra space before"

This is done for main.py only because the change is big enough even for a single file.

No logic have been changed and all works just fine on raspberry pi 3.

lechat · 2016-10-30T03:50:08Z

Those are the fixes for #21

renekliment · 2016-10-30T06:47:02Z

Hey there. Thanks! It's good to see someone caring about the style, knowing it matters 😉

I've got quite a PR locally I'm gonna push that I'd like to merge first. Would you mind if we merged it first and you'd rebase / resolve merge conflicts to get your PR merged?

lechat · 2016-10-30T07:00:22Z

I don't mind at all.

Speaking about style, why not adopting PEP8? ;) I took me some time to switch from normal python vim settings to this project's style and I am sure that this style is a big turn off for seasoned python developers, who would like to contribute.

renekliment · 2016-10-30T07:09:05Z

The current code is not PEP8 for historic reasons. As pointed out in the CONTRIBUTION.md, we'd like to go there. We've just been doing a lot of logic refactoring and I've wanted to do a cleanup (#21) that would rewrite the rest of the code to conform it afterwards. The reason is that a lot of code has been rewritten and there was no need to bother with code that was to be completely rewritten or removed.

The only thing where we'll go against PEP8 is tab indentation. If you agree with that, then cool. If you don't, I don't wanna start a tabs vs spaces flamewar 😄

renekliment · 2016-10-30T07:12:17Z

Also, very cool with the pylintrc file! Right now, there's three very silly and basic commands on Travis CI because the length of the file got me scared when I was investigating it. We need to move to a file, so we can set settings easily and people can run it locally.

lechat · 2016-10-30T12:09:06Z

The pylintrc is quite freestyle now. As I am not faminiliar with code yet, it is quite relaxed. The only thing matters in this file is which checks are disabled and I disabled a few checks.

I also want to change all print based logging to standard python logging, so the code will not be peppered with "if debug" statements. Is that ok?

renekliment · 2016-10-30T12:11:40Z

Could you post a link on the python logging thing you have in mind?

renekliment · 2016-11-01T15:12:34Z

If you want to clean up the whole thing, please set up the checks also in .travis.yaml to run pylint.

lechat · 2016-11-01T15:59:01Z

Standard pythin logging: https://docs.python.org/2/library/logging.html

Just to get rid of "if debug: print()"

renekliment · 2016-11-01T16:14:03Z

Yep, this is definitely the way to go. I see this as a new PR that comes after all the current PRs get merged. Feel free to open an issue for that so we keep track of it.

renekliment · 2016-11-04T07:08:13Z

src/alexapi/device_platforms/orangepi.py

-				and 'command' in self.__pconfig['long_press']
-				and len(self.__pconfig['long_press']['command']) > 0
-				and 'duration' in self.__pconfig['long_press']):
+		if ('long_press' in self.__pconfig and


So I'm not sure I like this. My way, each additional condition is responsible for adding and and I can take out every single one of them without affecting other lines. The and-at-the-end-of-the-line way seems silly this way. This is in other places too.

This is default PEP8 style enforced by pylint: http://legacy.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

But, as PEP8 says that it doesn't matter as long is the style is consistent across whole code, I will revert this.

Thank you! Please set up pylint to accept this.

renekliment · 2016-11-04T07:10:37Z

src/alexapi/device_platforms/raspberrypi.py


 		time.sleep(.5)  # time for the button input to settle down
-		while (GPIO.input(self.__pconfig['button']) == 0):
+		while GPIO.input(self.__pconfig['button'] == 0):


You've got a little error here with the parenthesis.

Yes, removed wrong one...

renekliment · 2016-11-04T20:42:25Z

.travis.yml

@@ -10,13 +10,9 @@ before_install:

 install:
  - pip install -r src/requirements.txt
-  - pip install fakeRPiGPIO
+  - pip install -r src/dev-requirements.txt


Ah yes, this is a better idea!

renekliment · 2016-11-04T20:44:13Z

.travis.yml

-  - pylint --errors-only --extension-pkg-whitelist=alsaaudio,pyA20 src/alexapi
-  - pylint --errors-only src/auth_web.py
+script:
+  - pylint --rcfile=pylintrc --extension-pkg-whitelist=alsaaudio,pyA20 src/*.py


If I'm correct:

we don't need --extension-pkg-whitelist=alsaaudio,pyA20 here anymore, since it's in the pylintrc file

this doesn't check the src/alexapi/ directory

lechat · 2016-11-05T03:23:06Z

There is a duplicated code in orangepi and raspberrypi. Perhaps that code needs to be moved into common ancestor for both devices.

renekliment · 2016-11-05T08:46:24Z

Yes, I haven't thought it through entirely. I'll do a bit of cleanup on the device_platform front after we merge the open stuff.

renekliment · 2016-11-05T08:47:45Z

src/alexapi/device_platforms/orangepi.py

@@ -3,7 +3,7 @@
 import os
 import threading

-from pyA20.gpio import gpio as GPIO
+from pyA20.gpio import gpio as GPIO   # pylint: disable=import-error


What do you need this for? The pyA20 library should be installed on dev machines as well as on Travis CI.

renekliment · 2016-11-05T08:48:34Z

src/alexapi/device_platforms/raspberrypi.py

@@ -64,7 +64,7 @@ def indicate_recording(self, state=True):
 	def indicate_playback(self, state=True):
 		GPIO.output(self.__pconfig['plb_light'], GPIO.HIGH if state else GPIO.LOW)

-	def detect_button(self, channel):


We actually need this. It's not used anywhere, but this is how the underlying C library calls it. It crashes if it's not there.

Then it is better to add that method with this signature to abstract class. And a comment about this in code, just in case somebody will wonder why it is there.

I'll do that in #73 then. Please put it back in the meantime.

I will do it this evening

renekliment · 2016-11-05T08:49:38Z

pylintrc

@@ -55,7 +55,7 @@ extension-pkg-whitelist=alsaaudio,pyA20
 # E1103: %s %r has no %r member (but some types could not be inferred) - fails to infer real members of types, e.g. in Celery
 # W0231: method from base class is not called - complains about not invoking empty __init__s in parents, which is annoying
 # R0921: abstract class not referenced, when in fact referenced from another egg
-disable=mixed-indentation,line-too-long,bad-continuation,missing-docstring,too-many-nested-blocks,too-many-branches,no-self-use,global-statement,too-many-statements
+disable=mixed-indentation,line-too-long,bad-continuation,missing-docstring,too-many-nested-blocks,too-many-branches,no-self-use,global-statement,too-many-statements,duplicate-code,relative-import


Where do we have mixed indentation? That could (and should) be fixed easily I believe.

Mixed identation is "tabs for indentation and spaces for alignment". You just said that tabs are staying... ;)

Damn, stupid python. I totally forgot about python stuff when I wrote the guide. Let's please do tabs only then - could you also edit the contribution guide?

Tabs only == rewriting indentation in all python files... I think that this is better to be done in separate pull request.

If I checked correctly, it's only a few places in main.py - the command line arguments and one line in orangepi.py. We can ignore tunein.py. It's an extenal thing that we need to pull new version of and stuff anyway.

renekliment · 2016-11-05T08:58:43Z

.travis.yml

@@ -15,4 +15,5 @@ install:
  - cp src/config.template.yaml src/config.yaml

 script:
-  - pylint --rcfile=pylintrc --extension-pkg-whitelist=alsaaudio,pyA20 src/*.py
+  - pylint --rcfile=pylintrc src/*.py src/alexapi/*.py


I believe we can squeeze these two lines into just pylint src/*.py src/alexapi

Nope, doesn't work. Pylint will only take one path...

It works for me. What does it give you?

It just silently skips second path for me. Add some unused import into raspbrerry_platform.py and run pylint with two paths - it will ignore this path.

https://gist.github.com/renekliment/6a8637deae834ea0d3fcb13e2a6c113e
It's not your branch, so it doesn't have the pylintrc file, but it should behave the same ... which it doesn't I guess.

renekliment · 2016-11-05T09:18:18Z

Also, please add
-e git+https://github.com/xtacocorex/CHIP_IO.git#egg=CHIP_IO into dev-requirements.txt
and CHIP_IO into whitelisted modules in pylintrc.

We're close to merging this! I'll just make you a mess / nightmare by merging #68 and we're almost there 😃

Thank you for doing this - it's gonna be much better quality code after we're done!

imports

renekliment · 2016-11-09T18:30:02Z

Please address my requests from my code review and I'm gonna test and merge this. We can then have a look at the logging stuff (yes, I've been spying in your repo 😄).

It's gonna take me some time to rework my PR so I'm gonna rebase on this later, when I've got that sorted out.

Thanks!

function, one line travis call

renekliment · 2016-11-10T14:26:04Z

So I gave this a brief testing and everything seems to work fine (as before).

We could probably fix a few easy things due to which pylint3 fails, but there's gonna be many more things to make sure it works under python3 correctly, so we do that when doing #7.

Great job! Merging!

renekliment · 2016-11-10T14:37:45Z

Oh, we still need to remove the spaces for indentation from CONTRIBUTING.md. I guess I'll just do that when updating docs when releasing v1.3.

lechat force-pushed the pythonize branch from bda97b3 to 2defffe Compare November 2, 2016 01:14

renekliment suggested changes Nov 4, 2016

View reviewed changes

lechat force-pushed the pythonize branch from a053ef1 to 5c00e3d Compare November 5, 2016 03:14

renekliment suggested changes Nov 5, 2016

View reviewed changes

renekliment added this to the 1.3 milestone Nov 5, 2016

lechat added 11 commits November 5, 2016 18:53

Cleaned up code

25400d8

Added pylint rc file

2374faa

Ignored vim swap files

3a5c0c1

Cleaned up the rest of code base

ed004d6

Changed code based on review

d9eb64f

Fixed or disabled pylint for the rest of codebase, changed travis config

eed8329

Requrements file for development/tests

1c83c28

Make sure that pylint runs over all code. Disaled checks for relative

a950c91

imports

Fixed code style

40660dc

Removed override check and non working duplication in travis

55de3eb

Rebased on master and cleaned up new code

cee16ef

lechat force-pushed the pythonize branch from 4056e27 to cee16ef Compare November 5, 2016 11:03

This was referenced Nov 5, 2016

Playback handlers + Queue #68

Merged

Do logging properly #75

Closed

Code review: returned mixed identation check, restored parameter for

08d13bc

function, one line travis call

renekliment approved these changes Nov 10, 2016

View reviewed changes

renekliment merged commit 82a9718 into alexa-pi:master Nov 10, 2016

lechat deleted the pythonize branch November 10, 2016 14:48

renekliment mentioned this pull request Nov 10, 2016

fix the scope parameter, better style #84

Merged

Code cleanup #67

Code cleanup #67

Conversation

lechat commented Oct 30, 2016

lechat commented Oct 30, 2016

renekliment commented Oct 30, 2016

lechat commented Oct 30, 2016

renekliment commented Oct 30, 2016

renekliment commented Oct 30, 2016

lechat commented Oct 30, 2016

renekliment commented Oct 30, 2016

renekliment commented Nov 1, 2016 • edited

lechat commented Nov 1, 2016

renekliment commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lechat commented Nov 5, 2016

renekliment commented Nov 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renekliment commented Nov 5, 2016 • edited

renekliment commented Nov 9, 2016

renekliment commented Nov 10, 2016

renekliment commented Nov 10, 2016

renekliment commented Nov 1, 2016 •

edited

renekliment commented Nov 5, 2016 •

edited