Fix Flake8 violations for files in the test/ directory #1096

LucyJimenez · 2022-03-03T23:30:07Z

Fix pep8 issues on:

test_util.py
test_subprocess.py

This reverts commit 2af36bf.

LucyJimenez · 2022-03-04T11:51:21Z

test/test_util.py

@@ -1,18 +1,15 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
-import io
-import locale
+import locale # noqa F401 imported but unused


The locale library is not used and in the code of the file it is not found anywhere else, but when I remove it I get an error in AppVeyor CI. Any idea why this happens?

Can you move this to it's own PR, so we can isolate and see the CI failure without other changes.

This is not addressed.

datapythonista · 2022-03-04T13:57:17Z

test/test_subprocess.py

@@ -34,7 +34,7 @@ def test_timeout():
 sys.stdout.flush()
 sys.stderr.flush()
 subprocess.call([sys.executable, "-c",
-    "import sys, subprocess; subprocess.call([sys.executable, '-c', 'import time; time.sleep(360)'])"])
+    "import sys, subprocess; subprocess.call([sys.executable, '-c', 'import time; time.sleep(360)'])"])  # noqa E501 line too long


I don't see any reason why this can't be splitted in multiple lines.

I'm not sure why it is not easy to split this line. I tried a couple of things, and it doesn’t work. For example:
Option 1:

Option 2:

Could you please help me?

This is Python code as a string. It needs to be valid Python code when converted to a .py file. And this is clearly not, as you have a multiline string with single quotes.

datapythonista · 2022-03-04T13:58:21Z

test/test_util.py

@@ -1,18 +1,15 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
-import io
-import locale
+import locale # noqa F401 imported but unused


Can you move this to it's own PR, so we can isolate and see the CI failure without other changes.

LucyJimenez · 2022-03-04T14:52:16Z

@datapythonista I'm going to slip this PR to get more info about the errors.

datapythonista · 2022-03-04T15:57:47Z

@datapythonista I'm going to slip this PR to get more info about the errors.

Not sure what you mean, but the line too long just needs to be split and for the locale import, you can just have a message that things fail if removed, and create and link an issue to check why.

…o issue1042_p4

LucyJimenez · 2022-03-08T10:53:17Z

@datapythonista The PR is ready for your review; thanks!

datapythonista · 2022-03-08T10:58:50Z

test/test_util.py

@@ -1,18 +1,15 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
-import io
-import locale
+import locale # noqa F401 imported but unused


This is not addressed.

datapythonista · 2022-03-08T11:03:45Z

test/test_subprocess.py

@@ -34,7 +34,8 @@ def test_timeout():
 sys.stdout.flush()
 sys.stderr.flush()
 subprocess.call([sys.executable, "-c",
-    "import sys, subprocess; subprocess.call([sys.executable, '-c', 'import time; time.sleep(360)'])"])
+    "import sys, subprocess; subprocess.call([sys.executable, '-c', \
+    'import time; time.sleep(360)'])"])


This is quite hard to read. The code itself is easy to read, we should split it in a way that makes things more clear.

I think I'd have the parameters of call in their own line: sys.executable, '-c', 'import time; time.sleep(360)'.

And I would avoid using a backslash, as this makes things harder to read, besides introducing spaces. I'd just have one string per line, which Python will automatically concatenate.

Thanks so much for all the explanations. It was enlightening, and I tried to apply your comments to get a better result.

LucyJimenez · 2022-03-08T16:07:05Z

@datapythonista The CI / test (pypy-3.8) has been running for ~50 minutes. Is it normal?

datapythonista

I will have a look at the pypy build if it happens again.

datapythonista · 2022-03-08T19:30:36Z

test/test_subprocess.py

-subprocess.call([sys.executable, "-c",
-    "import sys, subprocess; subprocess.call([sys.executable, '-c', 'import time; time.sleep(360)'])"])
+subprocess.call([sys.executable, "-c", "import sys, subprocess;"
+"subprocess.call([sys.executable, '-c', 'import time; time.sleep(360)'])"])


Not sure if this changed, but this is still not very readable. This is much more readable:

subprocess.call([ sys.executable, "-c", "import sys, subprocess; subprocess.call([sys.executable, '-c', 'import time; time.sleep(360)'])" ])

And if still too long:

subprocess.call([ sys.executable, "-c", "import sys, subprocess; subprocess.call(" "[sys.executable, -c', import time; time.sleep(360)'])" ])

Or something similar, where is clear which are the parameters of each function.

Thank you, I'll put it in the code.

This option works well, but is too long E501 line too long (101 > 99 characters)

subprocess.call([ sys.executable, "-c", "import sys, subprocess; subprocess.call([sys.executable, '-c', 'import time; time.sleep(360)'])" ])

The second option was implemented but it was not working.

subprocess.call([ sys.executable, "-c", "import sys, subprocess; subprocess.call(" "[sys.executable, -c', import time; time.sleep(360)'])" ])

@datapythonista what is the best thing to do in this case?

datapythonista · 2022-03-09T10:20:50Z

test/test_subprocess.py

+    sys.executable,
+    "-c",
+    "import sys, subprocess; subprocess.call("
+    "[sys.executable, -c', import time; time.sleep(360)'])"


Suggested change

"[sys.executable, -c', import time; time.sleep(360)'])"

"[sys.executable, '-c', import time; time.sleep(360)'])"

A quote is missing

@datapythonista I fix the quote missing, but it is still not working. Do you think I could use this code instead?

subprocess.call([sys.executable, "-c", "import sys, subprocess; subprocess.call(\ [sys.executable, '-c', 'import time; time.sleep(360)'])"])

The backslash is irrelevant, and as I mentioned, a discouraged practice in Python. There is another quote missing:

[sys.executable, '-c', import time; time.sleep(360)']

[sys.executable, '-c', 'import time; time.sleep(360)']

datapythonista

There are conflicts that need to be solved, and one comment.

datapythonista · 2022-03-09T16:37:42Z

test/test_util.py

@@ -1,18 +1,15 @@
 # Licensed under a 3-clause BSD style license - see LICENSE.rst
-import io
-import locale
+import locale # noqa F401 imported but unused see #1106


The errors in #1106 seem unrelated to this. Can you try to remove this again, and see if the error is relevant this time please.

I removed the import locale and there are errors for the CI / test (ubuntu-latest, pypy-3.7) (pull_request) and the CI / test (ubuntu-latest, pypy-3.8) (pull_request). The tests are passed for the test/test_subprocess.py and test/test_util.py files in both cases.

The AppVeyor is not presenting errors.

These failures seem unrelated. Can you just remove the locale import from this PR.

LucyJimenez · 2022-03-11T12:34:23Z

@datapythonista, I think now this PR is ready for the last review :)

datapythonista · 2022-03-11T12:39:41Z

Thanks @LucyJimenez

LucyJimenez added 10 commits March 3, 2022 15:37

fix pep8 issues

2af36bf

Revert "fix pep8 issues"

e734a38

This reverts commit 2af36bf.

Merge branch 'airspeed-velocity:master' into master

e557e18

Merge branch 'master' of https://github.com/LucyJimenez/asv

66f8914

fix flake8 issues

0ae46cc

fix flake8 issues

a7e2510

fix flake8 issues

8422067

fix setup

64f4830

fix flake8 issues

cbb477b

fix pep8 issues

9966c14

LucyJimenez marked this pull request as draft March 4, 2022 01:24

LucyJimenez added 8 commits March 3, 2022 20:48

revert changes

dbce660

remove import

344dd2a

remove import time

be5d5bb

remove import locale

288b6ec

remove import io

3b92c3b

fix pep8 issues related

7cc7462

fix pep8 issues related

0d205ca

check use of noqa

2ca3eb7

LucyJimenez commented Mar 4, 2022

View reviewed changes

LucyJimenez marked this pull request as ready for review March 4, 2022 11:51

LucyJimenez added 3 commits March 4, 2022 06:57

fix pep8 issue

795cbed

add pepe8 error

8b0bf85

add pepe8 name error

7eb1a8e

datapythonista reviewed Mar 4, 2022

View reviewed changes

LucyJimenez requested a review from datapythonista March 4, 2022 14:51

LucyJimenez added 2 commits March 7, 2022 10:40

Merge branch 'airspeed-velocity:master' into issue1042_p4

378f1e3

Merge branch 'issue1042_p4' of https://github.com/LucyJimenez/asv int…

7279e5c

…o issue1042_p4

LucyJimenez added 9 commits March 7, 2022 10:47

fix pepe8 issue on test_subprocess.py

5cdd452

fix pepe8 issue on test_subprocess.py

9d08108

fix pepe8 issue on test_subprocess.py

6001453

fix pepe8 issue on test_subprocess.py

229f941

fix pepe8 issue on test_subprocess.py

3945c31

fix pepe8 issue on test_subprocess.py

4e23ef9

Merge branch 'airspeed-velocity:master' into issue1042_p4

8ca68ef

fix pepe8 issue

67fc1ce

fix pepe8 issue

1635f5e

datapythonista reviewed Mar 8, 2022

View reviewed changes

LucyJimenez added 2 commits March 8, 2022 09:52

fix pepe8 issue and address issue

9af7f27

fix pepe8 issue

03d16d9

datapythonista reviewed Mar 8, 2022

View reviewed changes

fix pepe8 issue

edec5a6

datapythonista reviewed Mar 9, 2022

View reviewed changes

LucyJimenez added 2 commits March 9, 2022 07:23

fix quote

168387c

fix another quote missing

6728f44

LucyJimenez requested a review from datapythonista March 9, 2022 14:15

datapythonista reviewed Mar 9, 2022

View reviewed changes

LucyJimenez added 6 commits March 9, 2022 12:08

Merge branch 'master' into issue1042_p4

10492c6

Merge branch 'airspeed-velocity:master' into issue1042_p4

0c25537

remove import locale

3a20810

include locale import

8fc3fb0

add space before # noqa

a1bb28d

remove the locale import

25f49b5

datapythonista merged commit d92fc39 into airspeed-velocity:master Mar 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Flake8 violations for files in the test/ directory #1096

Fix Flake8 violations for files in the test/ directory #1096

LucyJimenez commented Mar 3, 2022 •

edited

Loading

LucyJimenez Mar 4, 2022

datapythonista Mar 4, 2022

datapythonista Mar 8, 2022

datapythonista Mar 4, 2022

LucyJimenez Mar 7, 2022 •

edited

Loading

datapythonista Mar 7, 2022

datapythonista Mar 4, 2022

LucyJimenez commented Mar 4, 2022

datapythonista commented Mar 4, 2022

LucyJimenez commented Mar 8, 2022

datapythonista Mar 8, 2022

datapythonista Mar 8, 2022

LucyJimenez Mar 8, 2022

LucyJimenez commented Mar 8, 2022

datapythonista left a comment

datapythonista Mar 8, 2022

LucyJimenez Mar 8, 2022

LucyJimenez Mar 9, 2022 •

edited

Loading

datapythonista Mar 9, 2022

LucyJimenez Mar 9, 2022

datapythonista Mar 9, 2022

datapythonista left a comment

datapythonista Mar 9, 2022

LucyJimenez Mar 10, 2022 •

edited

Loading

datapythonista Mar 10, 2022

LucyJimenez Mar 11, 2022

LucyJimenez commented Mar 11, 2022

datapythonista commented Mar 11, 2022

	"[sys.executable, -c', import time; time.sleep(360)'])"
	"[sys.executable, '-c', import time; time.sleep(360)'])"

Fix Flake8 violations for files in the test/ directory #1096

Fix Flake8 violations for files in the test/ directory #1096

Conversation

LucyJimenez commented Mar 3, 2022 • edited Loading

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

LucyJimenez Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucyJimenez commented Mar 4, 2022

datapythonista commented Mar 4, 2022

LucyJimenez commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucyJimenez commented Mar 8, 2022

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucyJimenez Mar 9, 2022 • edited Loading

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

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucyJimenez Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucyJimenez commented Mar 11, 2022

datapythonista commented Mar 11, 2022

LucyJimenez commented Mar 3, 2022 •

edited

Loading

LucyJimenez Mar 7, 2022 •

edited

Loading

LucyJimenez Mar 9, 2022 •

edited

Loading

LucyJimenez Mar 10, 2022 •

edited

Loading