BOM-1074 - Run black on edx-lint #108

feanil · 2019-12-02T19:24:18Z

No description provided.

nedbat · 2019-12-02T19:45:23Z

Seems like black doesn't know our line length is 120?

feanil · 2019-12-02T19:59:48Z

@nedbat yea, I had left it alone until I figured out if I wanted at pyproject.toml or not. But added it to the make targets for now.

This does the minimum amount neede to no longer test on python 2.7

When we run pylint in python 3.5 we get new errors and a bunch of previous disables are no longer necessary. This cleans up all of those issues.

nedbat

Don't mind me, just griping. The code used to be nice... :(

nedbat · 2019-12-02T22:15:23Z

edx_lint/cmd/amnesty.py

@@ -21,7 +21,8 @@
 )
 PYLINT_EXCEPTION_REGEX = re.compile(r"""\s*#\s*pylint:\s+disable=(?P<disables>[^#$]+?)(?=\s*(#|$))""")

-PylintError = namedtuple('PylintError', ['filename', 'linenum', 'error_code', 'error_name', 'function', 'error_msg'])
+PylintError = namedtuple("PylintError", ["filename", "linenum", "error_code", "error_name", "function", "error_msg"],)


Is there a logic to why there's a trailing comma now?

It looks like this started with the latest black release (19.10b0), the previous 19.3b0 doesn't do this. Seems to be an unintended side-effect of psf/black#826 , may be worth filing a bug for.

Should we revert the new trailing commas before merging this? Re-running Black with version 19.3b0 should fix it, not sure if that would make any other undesired changes.

nedbat · 2019-12-02T22:15:54Z

edx_lint/pylint/common.py

@@ -27,5 +28,5 @@ def usable_class_name(node):
    name = node.qname()
    for prefix in ["__builtin__.", "builtins.", "."]:
        if name.startswith(prefix):
-            name = name[len(prefix):]
+            name = name[len(prefix) :]


Probably no point asking why black inserted a space before the colon....

Details in psf/black#279 . Comes from the following part of PEP 8:

However, in a slice the colon acts like a binary operator, and should have equal amounts on either side (treating it as the operator with the lowest priority). In an extended slice, both colons must have the same amount of spacing applied. Exception: when a slice parameter is omitted, the space is omitted.

PEP 8 basically allows for either sequence[a:b] or sequence[a : b], but not sequence[a: b]. The second of those condenses to the above when the second parameter is missing. Granted, the code before the change is perfectly PEP 8 compliant also, I think they just didn't want to try to figure out under which conditions the space after a colon is ok to omit.

edx_lint/pylint/i18n_check.py

test/plugins/test_range_check.py

test/plugins/test_right_assert_check.py

jmbowman · 2019-12-03T18:11:21Z

edx_lint/cmd/amnesty.py

@@ -21,7 +21,8 @@
 )
 PYLINT_EXCEPTION_REGEX = re.compile(r"""\s*#\s*pylint:\s+disable=(?P<disables>[^#$]+?)(?=\s*(#|$))""")

-PylintError = namedtuple('PylintError', ['filename', 'linenum', 'error_code', 'error_name', 'function', 'error_msg'])
+PylintError = namedtuple("PylintError", ["filename", "linenum", "error_code", "error_name", "function", "error_msg"],)


It looks like this started with the latest black release (19.10b0), the previous 19.3b0 doesn't do this. Seems to be an unintended side-effect of psf/black#826 , may be worth filing a bug for.

jmbowman · 2019-12-03T18:12:17Z

edx_lint/cmd/amnesty.py

-            disabled=", ".join(sorted(error_names)),
-            tag=tag_str,
-        )
+        return u"  # {tag}pylint: disable={disabled}".format(disabled=", ".join(sorted(error_names)), tag=tag_str,)


The trailing comma here is also omitted under black 19.3b0.

jmbowman · 2019-12-03T18:15:07Z

.travis.yml

    env: TOXARG="-e pylint"
+  - python: "3.6"
+    env: TOXARG="-e black-test"


black not working under Python 3.5 is going to be annoying given that we'll probably be running 3.5 as our default dev environment in most repos for another few months.

Yea, I think it may point to us waiting to actually deploy this until we're on 3.6 and above. Especially since python 3.6 won't be available on devstack by default. I think this was a good learning experience but we may want to hold off on actually turning on black for everything until we're on a newer version of python. @nedbat @jmbowman thoughts?

jmbowman · 2019-12-03T18:28:27Z

edx_lint/pylint/common.py

@@ -27,5 +28,5 @@ def usable_class_name(node):
    name = node.qname()
    for prefix in ["__builtin__.", "builtins.", "."]:
        if name.startswith(prefix):
-            name = name[len(prefix):]
+            name = name[len(prefix) :]


Details in psf/black#279 . Comes from the following part of PEP 8:

However, in a slice the colon acts like a binary operator, and should have equal amounts on either side (treating it as the operator with the lowest priority). In an extended slice, both colons must have the same amount of spacing applied. Exception: when a slice parameter is omitted, the space is omitted.

PEP 8 basically allows for either sequence[a:b] or sequence[a : b], but not sequence[a: b]. The second of those condenses to the above when the second parameter is missing. Granted, the code before the change is perfectly PEP 8 compliant also, I think they just didn't want to try to figure out under which conditions the space after a colon is ok to omit.

jmbowman · 2019-12-03T18:34:22Z

requirements/dev.in

@@ -2,3 +2,4 @@

 tox
 tox-battery
+black


Since black doesn't work under Python 3.5, this will force dev environments to use Python 3.6+ . That's a little less than ideal if we want to keep running make upgrade under 3.5 since that's our primary target version for now.

tox.ini

feanil

The 3.6 min requirement is unfortunate and I'm leaning towards waiting till our dev envs meet it before we push this forward, what do you think @nedbat @jmbowman ?

nedbat · 2019-12-03T21:53:33Z

I am fine with postponing black indefinitely :)

jmbowman · 2019-12-05T14:54:43Z

tox.ini

    pylint --version
    coverage run -p -m pytest {posargs:}

 [testenv:coverage]
-envdir = {toxworkdir}/py27-pylint17
+envdir = {toxworkdir}/py35-pylint17


Should this and testenv:pylint below be updated to use pylint24 instead?

jmbowman · 2019-12-05T14:57:27Z

edx_lint/cmd/amnesty.py

@@ -21,7 +21,8 @@
 )
 PYLINT_EXCEPTION_REGEX = re.compile(r"""\s*#\s*pylint:\s+disable=(?P<disables>[^#$]+?)(?=\s*(#|$))""")

-PylintError = namedtuple('PylintError', ['filename', 'linenum', 'error_code', 'error_name', 'function', 'error_msg'])
+PylintError = namedtuple("PylintError", ["filename", "linenum", "error_code", "error_name", "function", "error_msg"],)


Should we revert the new trailing commas before merging this? Re-running Black with version 19.3b0 should fix it, not sure if that would make any other undesired changes.

We still have a lot of repos that are going to be running on 3.5, it would be really annoying for them if they needed a second venv just to run black. Especially because it might not always be possible in various dev/test environments. We'll just hold off on enforcing this till next year when we're on at least 3.6

Since we're dropping support for python 2.7 we can also drop support for the old version of pylint and pylint-django.

feanil force-pushed the feanil/black-it branch 3 times, most recently from 047beb0 to e752aee Compare December 2, 2019 19:42

feanil force-pushed the feanil/black-it branch from e752aee to 6381a38 Compare December 2, 2019 19:57

feanil requested a review from jmbowman December 2, 2019 20:27

feanil changed the title ~~Feanil/black it~~ BOM-1074 - Run black on edx-lint Dec 2, 2019

feanil added 4 commits December 2, 2019 17:09

Drop support for python 2.7 and add python 3.5

fb1ca60

This does the minimum amount neede to no longer test on python 2.7

Fix pylint issues.

29ccf17

When we run pylint in python 3.5 we get new errors and a bunch of previous disables are no longer necessary. This cleans up all of those issues.

Add black as a dev requirement.

a5dee49

Run make upgrade.

65f0d8c

feanil force-pushed the feanil/black-it branch from 6381a38 to 743e53e Compare December 2, 2019 22:09

nedbat reviewed Dec 2, 2019

View reviewed changes

jmbowman reviewed Dec 3, 2019

View reviewed changes

feanil commented Dec 3, 2019

View reviewed changes

feanil requested a review from jmbowman December 4, 2019 21:58

jmbowman reviewed Dec 5, 2019

View reviewed changes

feanil requested a review from jmbowman December 6, 2019 19:53

jmbowman approved these changes Dec 9, 2019

View reviewed changes

feanil and others added 7 commits December 9, 2019 11:14

Run black --line-length 120 . from the root of this repo.

2d955d5

Test to ensure black was already run.

6cf1d90

Use parens to tame an awkward blacking

b6f3083

More parens to make a black choice a little less worse

a705e38

Now that we're py3-only, use a set literal

e53e69c

Drop support for older versions of Pylint.

cc96245

Since we're dropping support for python 2.7 we can also drop support for the old version of pylint and pylint-django.

feanil force-pushed the feanil/black-it branch from 24c5aca to cc96245 Compare December 9, 2019 16:16

feanil merged commit 17c3bd0 into master Dec 9, 2019

feanil deleted the feanil/black-it branch December 9, 2019 16:20

regisb mentioned this pull request Oct 16, 2020

[BD-21] Fix requirements installation #128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BOM-1074 - Run black on edx-lint #108

BOM-1074 - Run black on edx-lint #108

feanil commented Dec 2, 2019

nedbat commented Dec 2, 2019

feanil commented Dec 2, 2019

nedbat left a comment

nedbat Dec 2, 2019

jmbowman Dec 3, 2019

jmbowman Dec 5, 2019

nedbat Dec 2, 2019

jmbowman Dec 3, 2019

jmbowman Dec 3, 2019

jmbowman Dec 3, 2019

jmbowman Dec 3, 2019

feanil Dec 3, 2019

jmbowman Dec 3, 2019

jmbowman Dec 3, 2019

feanil left a comment

nedbat commented Dec 3, 2019

jmbowman Dec 5, 2019

jmbowman Dec 5, 2019

BOM-1074 - Run black on edx-lint #108

BOM-1074 - Run black on edx-lint #108

Conversation

feanil commented Dec 2, 2019

nedbat commented Dec 2, 2019

feanil commented Dec 2, 2019

nedbat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feanil left a comment

Choose a reason for hiding this comment

nedbat commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment