Skip to content
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

Enable more linting #292

Open
wants to merge 1 commit into
base: master
from
Open

Enable more linting #292

wants to merge 1 commit into from

Conversation

@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Jul 26, 2016

WIP-ish PR. Interested to see if there are any parts that are objectionable.

I'm happy to split it into smaller chunks for different types of lint issues, with nice commit messages.

Also, I can add more fine tuning by implementing my own flake8-putty, which allows rules to be disabled per-file and even per-line without modifying the source, so new rules can be enforced for new code while existing lint errors in old code are ignored.


This change is Reviewable

@gsnedders
Copy link
Member

@gsnedders gsnedders commented Jul 26, 2016

In general, <3. Thanks for doing this! I'm pretty sure I'd rather not have some of the changes because I disagree with the lint, but I think that's a small number of the changes; will have a close look through tomorrow.

FWIW, there was some vague intent to just get rid of flake8-run.sh given it's pretty gratuitous now. Also, when it comes to pylint, if we're going to have it in Travis/tox we should probably pin it to one exact version to avoid breakage when it gets updated (this really goes for flake8 and its dependencies too, but I've far more experience of pylint changing things like the disable strings from version-to-version).

@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Jul 27, 2016

No worries. Im just waking up, so I'll focus on cleaning up only the actual provable user-facing bugs herein until I hear more from you.


import urllib.request
import urllib.error
import urllib.parse
import urllib.robotparser
import md5

This comment has been minimized.

@jayvdb

jayvdb Jul 28, 2016
Author Contributor

Testing this change turned into a mini project #294 .

@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Jul 28, 2016

I split off the CI and pylint changes to be #293.

@willkg willkg added this to the 1.0 milestone Oct 3, 2017
@willkg
Copy link
Contributor

@willkg willkg commented Nov 29, 2017

@jayvdb We've landed a bunch of changes, but I'm a little fuzzy on whether all the important things in this PR have been covered now or not. Are there outstanding things we want in this PR or can we close it out?

@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Nov 29, 2017

Quite a few parts are now simpler, but still heaps to do. I rebased and revised this last week, but was waiting for the other PRs to be merged, and was doing Google Code-in launch prep.
I'll push my revised version asap.

@willkg willkg removed this from the 1.0 milestone Dec 4, 2017
@willkg
Copy link
Contributor

@willkg willkg commented Dec 4, 2017

I want to get 1.0.0 out, so I'm going to drop this from the milestone.

Thank you for working on it! We can definitely land this in the future.

@hugovk
Copy link
Contributor

@hugovk hugovk commented Nov 8, 2019

At this point in time, I'd suggest just running Black and let it decide on the style choices.

@gsnedders
Copy link
Member

@gsnedders gsnedders commented Feb 26, 2020

@hugovk Let's try and minimise the number of open PRs when we do that (i.e., wait till after we've merged many of the currently open PRs), given it'll cause conflicts with everything, but yes, applying black seems sane. (Keeping this open in lieu of opening a new issue because I'm about to go to bed 🙃)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.