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

Fix pep8 violations #329

Merged
merged 12 commits into from
Jul 25, 2018
Merged

Fix pep8 violations #329

merged 12 commits into from
Jul 25, 2018

Conversation

lucaswiman
Copy link
Contributor

@tony @carljm

Problem

One of the checkboxes in the PR checklist ("Write PEP8 compliant code.") wasn't covered in #317, since there were many pep8 violations already present in the repo. I said I would fix flake8 violations in another pull request.

Solution

I added a flake8 tox environment which runs flake8 to enforce pep8-style code formatting. I've ignored the following errors:

E731 do not assign a lambda expression, use a def

Personally this one always struck me as sort of silly and pedantic. I don't think it improves code readability, and can sometimes detract from it. I can fix the violation of this rule if requested.

W503 line break before binary operator

I think this actually leads to less readable code in many cases, since violating this allows showing the operator connecting the two lines at the beginning of the line (emphasized) rather than the end (deemphasized). As before, I can fix the violation of this rule if requested.

E402 module level import not at top of file

This seems to have been a false positive. I think flake8 dislikes this conditional import, but it looks fine to me:

if django.VERSION >= (1, 9, 0):
from django.db.models.functions import Now
now = Now()
else:
from django.utils.timezone import now
from model_utils.managers import QueryManager, SoftDeletableManager
from model_utils.fields import AutoCreatedField, AutoLastModifiedField, \
StatusField, MonitorField

If requested, I can either exclude this particular set of imports, or alter the ordering of execution, though it doesn't seem like a win for code clarity.

E501 line too long

There are a large number of violations of the 80 character line length limit, which argues against fixing this. In particular, it would likely introduce difficult merge conflicts with most of the open pull requests.

One option would be to pick a longer limit, like the 88 characters preferred by Black, or some other higher value like 100 or 120. The longest line is 114 characters, so choosing 120 would require no code changes. Choosing 100 would require few code changes (6 lines).

@lucaswiman lucaswiman requested review from tony and carljm July 2, 2018 21:58
@codecov
Copy link

codecov bot commented Jul 2, 2018

Codecov Report

Merging #329 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #329   +/-   ##
=======================================
  Coverage   98.42%   98.42%           
=======================================
  Files           2        2           
  Lines         254      254           
=======================================
  Hits          250      250           
  Misses          4        4
Impacted Files Coverage Δ
model_utils/choices.py 100% <ø> (ø) ⬆️
model_utils/tracker.py 97.86% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16dec4d...c53b19e. Read the comment docs.

@lucaswiman
Copy link
Contributor Author

Seems like there are no objections, and this seems like an incremental improvement, so I'm going to merge this. The line length thing can be figured out later.

@lucaswiman lucaswiman merged commit bf1adce into jazzband:master Jul 25, 2018
@lucaswiman lucaswiman deleted the fix-pep8-violations branch July 25, 2018 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant