-
Notifications
You must be signed in to change notification settings - Fork 216
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 #29
pep8 #29
Conversation
Caused by the location of the copyright notices
fixes W0611 warnings
Also removed unused imports and updated travis. We are now down to these issues:
|
After tidying up most whitespace issues we are down to these issues:
|
For these linter issues: E302, E303, E305, E111, E114, E125, E127, E128, E129
for linter issue E722
for linter E128
for linter issue E129
py3 lint undefined name 'unicode'
The warnings have been reduced to this issue:
The repeated code could be abstracted into another function to resolve this, but it's safe to ignore. |
Hi @brennv , guess it's time for me to nudge somebody at LinkedIn... 😀 @RiteshMaheshwari , you're the only one who's been responding, but I'm more than happy to ping somebody else. It would be great to get this PR (plus the two others) reviewed and merged. |
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.
LGTM
Any time when suggesting pep8 changes we are reminded of this. With that in mind, I'd like to suggest some aesthetic changes for convention and maintainability. In our Travis runs we check pep8 compliance as the last step. The last results before this PR are shown here:
https://travis-ci.org/linkedin/luminol/jobs/273116296#L663
This first commit serves only to make imports more explicit. It resolves all the warnings that read:
W0401 'from luminol.constants import *' used; unable to detect undefined names [pyflakes]
I'd like to add additional commits to this PR if folks are open to it.