-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP8 : ffmpeg_reader #11
Conversation
Ach, I am working on this file right now... Plus, I agree with the spacing correction, but for the rest, lines such as
are perfectly valid in my opinion, I don't think they need to be exploded into 4 lines. And I found a typo in your new lines: So I am not merging this one, sorry, but thanks ! PEP8 is indead a problem in the current state of the code. |
fixed the typo. I find the explicit |
Still not convinced by the conditional if, but the PEP8 corrections are worth the merge. Thanks ! |
Its a bit late now that you have already pushed the button, but for future reference, if you pull my branch down to your local machine and then do an interactive merge. You could revert the changes you don't like in the merge commit and then push the merge commit back up to the github master. Because my commit would be in the history (as you changed the code in the merge commit) github will auto-magically close the PR and you don't get the changes you don't want. It will also take you less time than it took me to write this comment to just revert it with a new commit ;) |
Ok, thanks for the tip ! Le 04/02/2014 17:57, Thomas A Caswell a écrit :
|
Hey ! In case you are interested, I changed a few lines in ffmpeg_reader to See the changes here: 252f175#diff-a49583100a493b6fe1c05b48824e4a3c Le 04/02/2014 17:57, Thomas A Caswell a écrit :
|
No description provided.