Applied isort and black to project #180
Conversation
Codecov Report
@@ Coverage Diff @@
## master #180 +/- ##
=======================================
Coverage 94.25% 94.25%
=======================================
Files 29 29
Lines 1270 1270
=======================================
Hits 1197 1197
Misses 73 73
Continue to review full report at Codecov.
|
analytical/templatetags/crazy_egg.py
Outdated
| '<script type="text/javascript" src="{placeholder_url}">' | ||
| "</script>".format( | ||
| placeholder_url="//dnn506yrbagrg.cloudfront.net/pages/scripts/" | ||
| "%(account_nr_1)s/%(account_nr_2)s.js" | ||
| ) |
There was a problem hiding this comment.
I can't help but saying, don't you feel this is harder to read? 😟 Man, Black, a hard time getting used to your style.
There was a problem hiding this comment.
I rewrote this a little bit, there's a lot going on here. Hopefully the way I have refactored is correct, and is a bit more readable.
There was a problem hiding this comment.
The tests will hopefully prove that your refactoring was successful! 😄 🤞
| @@ -39,11 +42,11 @@ | |||
|
|
|||
| VARIABLE_CODE = '_paq.push(["setCustomVariable", %(index)s, "%(name)s", "%(value)s", "%(scope)s"]);' # noqa | |||
| IDENTITY_CODE = '_paq.push(["setUserId", "%(userid)s"]);' | |||
| DISABLE_COOKIES_CODE = '_paq.push([\'disableCookies\']);' | |||
| DISABLE_COOKIES_CODE = "_paq.push(['disableCookies']);" | |||
There was a problem hiding this comment.
That one I like! But, yeah, that was an easy one.
| IDENTITY_CODE % {'userid': userid}, | ||
| )) | ||
| variables_code = chain( | ||
| variables_code, (IDENTITY_CODE % {"userid": userid},) |
There was a problem hiding this comment.
Wow! This is horribly difficult to understand!
| MIDDLEWARE_CLASSES = ( | ||
| 'django.middleware.common.CommonMiddleware', | ||
| 'django.middleware.csrf.CsrfViewMiddleware', | ||
| "django.middleware.common.CommonMiddleware", | ||
| "django.middleware.csrf.CsrfViewMiddleware", | ||
| ) |
There was a problem hiding this comment.
Oh! We should really convert this to the new-style MIDDLEWARE, e.g.
MIDDLEWARE = [
"django.middleware.common.CommonMiddleware",
"django.middleware.csrf.CsrfViewMiddleware",
]There was a problem hiding this comment.
This one sounds like it should be a separate PR? 🤔
There was a problem hiding this comment.
Weh-eh-ell, that sounds like that should have been done earlier! 😏
Didn't anyone write a linter for that yet? Maybe we should extend flake8-django to check on a list being used for MIDDLEWARE and INSTALLED_APPS.
| @@ -8,6 +8,7 @@ | |||
| from django.http import HttpRequest | |||
| from django.template import Context | |||
| from django.test.utils import override_settings | |||
| from utils import TestCase | |||
There was a problem hiding this comment.
All these utils imports are really "local" imports. I hope we won't get bitten by them, one day. It may make sense to convert all them to relative imports.
There was a problem hiding this comment.
This sounds like a separate PR to me?
|
I see the project has flake8 line length set at 100. I'm not sure how you feel about letting black use this as well? I'll have a look again through the project and move a few commas about, that may help a little. |
Do you intend, instead of the default 88? I don't know. I think I picked the |
We want to make the sort order of imports uniform across all modules, and so do we want to unify the coding style as such.
This is a follow-up of #152 (comment).