amirstm commented 8 months ago

Hey. There is an issue with autopep8 and python3.12 that autopep8 tries to format the content inside the f-strings. For instance connector = f"socks5://{user}:{password}:{url}:{port}" gets formatted to: connector = f"socks5: //{user}: {password}: {url}: {port}"

The following is another sample:

razvan commented 8 months ago

It even corrupts sources by adding newlines to fstrings, as can be tested with: https://github.com/stackabletech/image-tools/pull/6/files#diff-cdc0846192c47ce22d8f9a51ee3c0ed9847a222827f12d00a89569b815d92e57R177

supakeen commented 7 months ago

I'm seeing the same behavior where it's corrupting f-strings by adding newlines inside replacements. Using autopep8 2.0.4 and Python 3.12.0.

autopep8: commands[0]> bash -c 'python -m autopep8 --diff --max-line-length 120 -a -a -a -j0 -r --exit-code osbuild/ assemblers/* devices/* inputs/* mounts/* runners/* sources/* stages/*.* stages/test/*.py tools/'
--- original/stages/org.osbuild.debug-shell
+++ fixed/stages/org.osbuild.debug-shell
@@ -31,7 +31,8 @@

     unit = f"""
 [Unit]
-Description=Early root shell on {tty} FOR DEBUGGING ONLY
+Description=Early root shell on {
+        tty} FOR DEBUGGING ONLY
 DefaultDependencies=no
 IgnoreOnIsolate=yes
 ConditionPathExists={tty}

bneradt commented 7 months ago

I don't mean to come across as alarmist, but this is a serious bug. The https://github.com/apache/trafficserver project relies upon autopep8 to format its Python files, and this single issue keeping us from moving to fedora:39 because its system version of Python is 3.12. We use a pre-commit hook to block any commits that fail autopep8, so Apache Traffic Server devs currently cannot upgrade to fedora:39. Can the maintainer (@hhatto) please comment on the status of this issue and provide an ETA for the landing of its fix?

Thank you for this tool. It has been helpful to the Apache Traffic Server project.

Kind regards, Brian

hhatto commented 7 months ago

There are two issues to be discussed and they are listed in order.

1. First Issue

First, @amirstm 's example is incorrect.

$ echo connector = f"socks5://{user}:{password}:{url}:{port}"
connector = fsocks5://{user}:{password}:{url}:{port}
$ echo connector = f"socks5://{user}:{password}:{url}:{port}" | autopep8 -
connector = fsocks5: // {user}: {password}: {url}: {port}

The echo command is not outputting the correct Python f-string example because you did not specify a single quote as an argument. I recognize the following as correct examples In this case, there is no particular problem with the operation of autopep8.

$ echo 'connector = f"socks5://{user}:{password}:{url}:{port}"'
connector = f"socks5://{user}:{password}:{url}:{port}"
$ echo 'connector = f"socks5://{user}:{password}:{url}:{port}"' | autopep8 -
connector = f"socks5://{user}:{password}:{url}:{port}"

environment:

$ python -V
Python 3.12.0
$ autopep8 --version
autopep8 2.0.4 (pycodestyle: 2.11.1)

2. Second Issue

@razvan @supakeen @bneradt Thanks for reporting.

https://github.com/hhatto/autopep8/issues/712#issuecomment-1768481871 https://github.com/hhatto/autopep8/issues/712#issuecomment-1812082927 https://github.com/hhatto/autopep8/issues/712#issuecomment-1821893942

Second, a line break is inserted at the point after the { in the above f-string where the variable is specified. This is a surprise to autopep8 users. We believe this behavior needs to be improved. One idea is to have f-string not change any line breaks, etc.

If you have an opinion on this behavior, please let us know.

Thanks

supakeen commented 7 months ago

@hhatto correct, I personally feel like autopep8 should likely leave any indentation (including newlines) in multi-line strings alone.

There might be some discussion to be had about things that are expected to be formatted inside { and } in f-strings, but adding a newline isn't :)

bneradt commented 7 months ago

@hhatto: Thanks for the reply.

Just in case it's helpful, autopep8 on Python 3.12 greatly changes the white space within format strings, not only the addition of newlines. It adds or subtracts a variety of types of white space in various places. Here's some samples of a run of it on the apache/trafficserver code base:

Removing white space

diff --git a/tests/gold_tests/bad_http_fmt/bad_http_fmt.test.py b/tests/gold_tests/bad_http_fmt/bad_http_fmt.test.py
index 8e84e18e1..059971175 100644
--- a/tests/gold_tests/bad_http_fmt/bad_http_fmt.test.py
+++ b/tests/gold_tests/bad_http_fmt/bad_http_fmt.test.py
@@ -45,7 +45,7 @@ ts.Disk.ip_allow_yaml.AddLines([
     '    action: allow',
     '    methods:',
     '      - GET',
-    f'      - {random_method}',
+    f' - {random_method}',
 ])

Adding spaces around colons:

diff --git a/tests/gold_tests/autest-site/trafficserver.test.ext b/tests/gold_tests/autest-site/trafficserver.test.ext
index 1ebf39e0d..59de90f49 100755
--- a/tests/gold_tests/autest-site/trafficserver.test.ext
+++ b/tests/gold_tests/autest-site/trafficserver.test.ext
@@ -409,9 +409,9 @@ def MakeATSProcess(obj, name, command='traffic_server', select_ports=True,
             port_str += " {ssl_port}:quic {ssl_portv6}:quic:ipv6".format(
                 ssl_port=p.Variables.ssl_port, ssl_portv6=p.Variables.ssl_portv6)
         if enable_proxy_protocol:
-            port_str += f" {p.Variables.proxy_protocol_port}:pp {p.Variables.proxy_protocol_portv6}:pp:ipv6"
+            port_str += f" {p.Variables.proxy_protocol_port}: pp {p.Variables.proxy_protocol_portv6}: pp: ipv6"
             if enable_tls:
-                port_str += f" {p.Variables.proxy_protocol_ssl_port}:pp:ssl {p.Variables.proxy_protocol_ssl_portv6}:pp:ssl:ipv6"
+                port_str += f" {p.Variables.proxy_protocol_ssl_port}: pp: ssl {p.Variables.proxy_protocol_ssl_portv6}: pp: ssl: ipv6"
         #p.Env['PROXY_CONFIG_HTTP_SERVER_PORTS'] = port_str
         p.Disk.records_config.update({
             'proxy.config.http.server_ports': port_str,

Addition of newlines

You mentioned this above:

@@ -73,8 +74,9 @@ tr = Test.AddTestRun()
 tr.Processes.Default.StartBefore(ts)
 tr.Processes.Default.Command = (
     server_cmd(1) +
-    fr" ; printf 'GET {random_path}HTTP/1.0\r\n" +
-    fr"Host: localhost:{Test.Variables.server_port}\r\n" +
+    fr"
+printf 'GET {random_path}HTTP/1.0\r\n" +
+    fr"Host: localhost: {Test.Variables.server_port}\r\n" +
     r"X-Req-Id: 0\r\n\r\n'" +
     f" | nc localhost {ts.Variables.port} >> client.log" +
     " ; echo '======' >> client.log"

Here's a particularly extreme example:

diff --git a/tests/gold_tests/cache/background_fill.test.py b/tests/gold_tests/cache/background_fill.test.py
index 56125a492..c0d02fbb4 100644
--- a/tests/gold_tests/cache/background_fill.test.py
+++ b/tests/gold_tests/cache/background_fill.test.py
@@ -58,7 +58,7 @@ class BackgroundFillTest:
                 "dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key")

             self.ts[name].Disk.records_config.update({
-                "proxy.config.http.server_ports": f"{self.ts[name].Variables.port} {self.ts[name].Variables.ssl_port}:ssl",
+                "proxy.config.http.server_ports": f"{self.ts[name].Variables.port} {self.ts[name].Variables.ssl_port}: ssl",
                 "proxy.config.http.background_fill_active_timeout": "0",
                 "proxy.config.http.background_fill_completed_threshold": "0.0",
                 "proxy.config.http.cache.required_headers": 0,  # Force cache
@@ -111,15 +111,41 @@ class BackgroundFillTest:
         tr = Test.AddTestRun()
         self.__checkProcessBefore(tr)
         tr.Processes.Default.Command = f"""
-curl -X PURGE --http1.1 -vs http://127.0.0.1:{self.ts['for_httpbin'].Variables.port}/drip?duration=4;
-timeout 2 curl --http1.1 -vs http://127.0.0.1:{self.ts['for_httpbin'].Variables.port}/drip?duration=4;
+curl -X PURGE --http1.1 -vs http: //127.0.0.1: {self.ts['for_httpbin'].Variables.port}/drip?duration=4;
+timeout 2 curl --http1.1 -vs http://127.0.0.1:{self.ts['for_httpbin         '                                                        ]
+        .
+                                               V
+                                               a
+                                               r
+                                               i
+                                               a
+                                               b
+                                               l
+                                               e
+                                               s
+                                               .
+                                               p
+                                               o
+                                               r
+                                               t
+                                               } /
+        d
+        r
+        i
+        p
+        ?
+        d
+        u
+        r
+        a
+        t
+        i
+        o
+        n
+        =
+        4
 sleep 4;

Removing newlines

In addition to additional newlines, it seems to also sometimes remove newlines:

-curl --http1.1 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?duration=4
-"""
-        tr.Processes.Default.ReturnCode = 0
-        tr.Processes.Default.Streams.stderr = "gold/background_fill_1_stderr.gold"
-        self.__checkProcessAfter(tr)
-
+curl --http1.1 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?d uration=4 """         tr.Processes.Default.ReturnCode = 0         tr.Processes.Default.Streams.stderr = "gold/background_fill_1_stderr.gold"         self.__checkProcessAfter(tr)

Adding trailing white space

Note that the previous patch actually adds a trailing space at the end, which may not render in github.

miguelmartins3 commented 5 months ago

Similar issue in this snippet of code: Hope it helps to troubleshoot or testing.

TheKangaroo commented 4 months ago

I see the same problem with the newline after { in f strings but only with the pre-commit v2.0.4 hook. If run autopep8 v2.0.4 from the CLI everything seems to be okay. I have the following line in my code:

url=f'https://gitlab.example.example.example.com/api/v4/projects/{project_id}'

With the autopep8 CLI (just the whitespaces will be fixed, e.g. what I would expect):

url = f'https://gitlab.example.example.example.com/api/v4/projects/{project_id}'

With autopep8 pre-commit (the linebreak after { will be inserted):

url = f'https://gitlab.example.example.example.com/api/v4/projects/{
    project_id}'

My pre-commit config is:

  - repo: https://github.com/hhatto/autopep8
    rev: v2.0.4
    hooks:
      - id: autopep8

zcutlip commented 4 months ago

I see the same problem with the newline after { in f strings but only with the pre-commit v2.0.4 hook. If run autopep8 v2.0.4 from the CLI everything seems to be okay. I have the following line in my code:

I'm wondering, is it possible you're running the CLI within a virtualenv with a different version of python?

If I run it from the CLI with my default python, 3.12, it still breaks my f-strings. If I have autopep8 installed in a project's virtualenv with python 3.11 there's no problem

zcutlip commented 4 months ago

Edit: of course this only applies to Mac, and in particular homebrew python, but hopefully it's helpful

If you came here like me in hopes of a workaround, this seems to be working for me:

check if 'brew' exists and if the default python3 version is 3.12, and if so, add python 3.11 to start of $PATH

if _command_exists "brew";
then
    # HACK: autopep8 is currently broken on python 3.12
    # temporarily set python 3.11 at start of PATH
    # https://github.com/hhatto/autopep8/issues/712
    if python3 --version | grep -E '^Python 3\.12' >/dev/null;
    then
        PATH="$(brew --prefix python@3.11)"/libexec/bin:$PATH
        export PATH
    fi
fi

mvo5 commented 4 months ago

One interesting find is that it seems to be virtualenv inside py312 that is causing the issues. I was trying to write a test case for the (excellent btw) tests that autopep8 has and struggled and then noticed:

$ .tox/autopep8/bin/python --version
Python 3.12.2
$ .tox/autopep8/bin/python -m autopep8 --version
autopep8 2.0.4 (pycodestyle: 2.10.0)
$ .tox/autopep8/bin/python -m autopep8 --diff -r ./test/
/usr/lib/python3.12/site-packages/autopep8.py:182: DeprecationWarning: lib2to3 package is deprecated and may not be able to parse Python 3.10+
  from lib2to3.pgen2 import tokenize as lib2to3_tokenize
...
--- original/./test/mod/test_util_checksum.py
+++ fixed/./test/mod/test_util_checksum.py
@@ -30,5 +30,5 @@
     tempfile.flush()

     digest = TEST_RESULT[algorithm]
-    full_digest = f"{algorithm}:{digest}"
+    full_digest = f"{algorithm}: {digest}"
     assert checksum.verify_file(tempfile.name, full_digest), "checksums mismatch"

which looks clearly incorrect. However with the non-virtualenv version:

$ python3 --version
Python 3.12.2
$ python3 -m autopep8 --version
autopep8 2.0.4 (pycodestyle: 2.10.0)
/usr/lib/python3.12/site-packages/autopep8.py:182: DeprecationWarning: lib2to3 package is deprecated and may not be able to parse Python 3.10+
  from lib2to3.pgen2 import tokenize as lib2to3_tokenize
$

so here no diff is shown which is expected.

TheKangaroo commented 4 months ago

I'm wondering, is it possible you're running the CLI within a virtualenv with a different version of python? If I run it from the CLI with my default python, 3.12, it still breaks my f-strings. If I have autopep8 installed in a project's virtualenv with python 3.11 there's no problem

I just checked an my autopep8 installation is via brew, which wraps autopep8 in python 3.11, so no wonder it works as expected:

cat /opt/homebrew/bin/autopep8
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: /opt/homebrew/bin/autopep8
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ #!/opt/homebrew/opt/python@3.11/bin/python3.11
   2   │ # -*- coding: utf-8 -*-
   3   │ import re
   4   │ import sys
   5   │ from autopep8 import main
   6   │ if __name__ == '__main__':
   7   │     sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])

For the breaking part, I think pre-commit always runs python in a virtual env, so yes, this matches the observed behaviour of problems with python 3.12 in a virtual env.

supakeen commented 4 months ago

@mvo5 That's a very interesting observation especially as you have the same versions of Python installed in the virtual environment and outside of it. Could you show the output of pipdeptree (you'll have to install it, the package is called pipdeptree as well) in both the virtual environment and outside of it for autopep8?

mvo5 commented 4 months ago

AFAICT the output inside the virtualenv and outside are identical (diff shows no difference). It an interesting issue, I really wonder how the fact that it runs inside/outside a virtualenv can have this effect (and why only on py312).

hruskape commented 3 months ago

I have been hitting similar problem like @mvo5 . I have had identical version in use autopep8 2.0.4 (pycodestyle: 2.10.0). For me solution was to upgrade pycodestyle to 2.11.1 which is bringing python3.12 support [1].

[1] https://github.com/PyCQA/pycodestyle/blob/main/CHANGES.txt

mvo5 commented 3 months ago

I have been hitting similar problem like @mvo5 . I have had identical version in use autopep8 2.0.4 (pycodestyle: 2.10.0). For me solution was to upgrade pycodestyle to 2.11.1 which is bringing python3.12 support [1].

[1] https://github.com/PyCQA/pycodestyle/blob/main/CHANGES.txt

Thanks, that is amazing. I think that unblocks us now.

zcutlip commented 3 months ago

will there be a new release of autopep8 in order to trigger an update of pycodstyle?

hhatto commented 3 months ago

autopep8 v2.1.0 has been released. version 2.1.0 now supports pycodestyle v2.11.0 and above.

jngrad commented 2 months ago

The autopep8 v2.1.0 bugfix doesn't seem cover a few edge cases with pycodestyle 2.11.0 and 2.11.1:

high indentation levels with short strings:

$ code_sample="if 1:\n    if 1:\n        if 1:\n            err_msg = rf'provided index [{str(n)[1:-1]}] is out of range for shape [{str(list(shape))[1:-1]}]'"
$ echo -e "${code_sample}"
if 1:
  if 1:
      if 1:
          err_msg = rf'provided index [{str(n)[1:-1]}] is out of range for shape [{str(list(shape))[1:-1]}]'
$ echo -e "${code_sample}" | autopep8 -
if 1:
  if 1:
      if 1:
          err_msg = rf'provided index [{
              str(n)[1:-1]}] is out of range for shape [{str(list(shape))[1:-1]}]'

low indentation levels with long strings:

$ code_sample="if 1:\n    err_msg = f'please provide a test name as argument, like {prefix}lb_cpu__p3m_cpu (got {sys.argv})'"
$ echo -e "${code_sample}"
if 1:
  err_msg = f'please provide a test name as argument, like {prefix}lb_cpu__p3m_cpu (got {sys.argv})'
$ echo -e "${code_sample}" | autopep8 -
if 1:
  err_msg = f'please provide a test name as argument, like {
      prefix}lb_cpu__p3m_cpu (got {sys.argv})'

running with the --aggressive mode:

$ code_sample="class A:\n    def __str__(self):\n        return f'{self.__class__.__name__}({self.selected_mode.__class__.__name__})'"
$ echo -e "${code_sample}"
class A:
  def __str__(self):
      return f'{self.__class__.__name__}({self.selected_mode.__class__.__name__})'
$ echo -e "${code_sample}" | autopep8 -
class A:
  def __str__(self):
      return f'{self.__class__.__name__}({self.selected_mode.__class__.__name__})'
$ echo -e "${code_sample}" | autopep8 --aggressive -
class A:
  def __str__(self):
      return f'{self.__class__.__name__}({
          self.selected_mode.__class__.__name__})'

Environment:

$ python -V
Python 3.12.3
$ autopep8 --version
  autopep8 2.1.0 (pycodestyle: 2.11.1)
$ cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=24.04
DISTRIB_CODENAME=noble
DISTRIB_DESCRIPTION="Ubuntu Noble Numbat (development branch)"

I can drop the --aggressive flag, since there is a proposal to remove it. Yet the other edge cases remain problematic. While Python 3.12 allows line breaks in f-strings (3.12 release notes, section PEP 701: Syntactic formalization of f-strings, paragraph Multi-line expressions and comments), my project supports multiple Python releases, and Python 3.10 and 3.11 raise SyntaxError: unterminated string literal. As a workaround, one can add # nopep8 at the end of impacted f-strings to avoid formatting by autopep8.

zcutlip commented 2 months ago

Can confirm. Several format strings in pyonepassword are getting wrapped per PEP 701. This breaks python 3.9-3.11 compatibility.

I've added # nopep8, but would be nice if it was possible to disable these fixes in a config, or to specify a minimum python version compatibility mode

hhatto / autopep8

Problem with formatting f-strings on python3.12 #712