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

autopep double escape in multiline string literals #618

Open
c0c0n3 opened this issue Jan 31, 2022 · 9 comments
Open

autopep double escape in multiline string literals #618

c0c0n3 opened this issue Jan 31, 2022 · 9 comments
Assignees

Comments

@c0c0n3
Copy link
Member

c0c0n3 commented Jan 31, 2022

Describe the bug

With our linting configuration, it sometime autopep escapes escape chars that it shouldn't---see this comment to #499 about it.

To Reproduce

Install autopep and run the command line in lint.py.sh

$ cd ngsi-timeseries-api
$ pipenv install autopep8
$ pipenv shell
$ autopep8 --exit-code --recursive --in-place --aggressive --aggressive .

Git should report that timescale-container/quantumleap-db-setup.py changed. If you do a diff you should see a \ character being escaped. That shouldn't happen I think b/c the string in question is a multiline string literal?

$ git diff timescale-container/quantumleap-db-setup.py

@@ -205,7 +205,7 @@ CREATE DATABASE ${db_name}
     OWNER ${db_user}
     ENCODING 'UTF8';
 
-\connect ${db_name}
+\\connect ${db_name}

Expected behavior

Not sure if it's autopep at fault or backslashes should actually always be escaped. If you run the following Python code

print('''
      \connect
      ''')

the characters \connect get printed on stdout. But the same happens if you run

print('''
      \\connect
      ''')

Notice the double backslash...

Environment

autopep8 version: 1.6.0

@c0c0n3 c0c0n3 self-assigned this Jan 31, 2022
@chicco785
Copy link
Contributor

@c0c0n3 this is not happening for the github workflow afaik, maybe we have just to adjust something in the lint script?

@c0c0n3
Copy link
Member Author

c0c0n3 commented Jan 31, 2022

@chicco785 what autopep8 version do we have in CircleCI? I used the latest 1.6.0, maybe that doesn't happen in older versions? we could also look into what the --aggressive flag does...

@c0c0n3
Copy link
Member Author

c0c0n3 commented Jan 31, 2022

@chicco785 or it could be \ should always be escaped. Check this out:

print('''
      \tonnect
      ''')

outputs: onnect. So my guess is that the following outputs \connect by accident

print('''
      \connect
      ''')

simply because there's no \c escape sequence? We need to dig deeper...

@chicco785
Copy link
Contributor

@chicco785 what autopep8 version do we have in CircleCI? I used the latest 1.6.0, maybe that doesn't happen in older versions? we could also look into what the --aggressive flag does...

linting is performed by github workflows, not in circle-ci:
https://github.com/orchestracities/ngsi-timeseries-api/blob/master/.github/workflows/lint.yml

still from the workflow, I can't infer the autopep version.

@chicco785
Copy link
Contributor

here is the answer:
https://github.com/peter-evans/autopep8/blob/master/requirements.txt#L1

1.5.4

@c0c0n3
Copy link
Member Author

c0c0n3 commented Jan 31, 2022

So it's a problem (or feature?) with autopep v1.6. If I try linting w/ v 1.5.4

$ cd ngsi-timeseries-api
$ pipenv install autopep8==1.5.4
$ pipenv shell
$ autopep8 --exit-code --recursive --in-place --aggressive --aggressive .

git says nothing changed. In other words, autopep 1.5.4 does not escape \connect in timescale-container/quantumleap-db-setup.py.

@chicco785
Copy link
Contributor

I think the first thing we need to understand is if pep 1.6 is right or not :) Then let's handle accordingly.

@c0c0n3 c0c0n3 mentioned this issue Jul 20, 2022
12 tasks
@Ravisaketi
Copy link
Contributor

Ravisaketi commented Sep 15, 2022

Hi, @c0c0n3 I analyzed this issue and my understanding was autopep version in lint.py.sh as pip install --upgrade autopep8. when we run the lint.py.sh it will automatically upgrade autopep version to the latest. As of now, we have the latest v1.7.0 actually we face the same issue in both v1.6.0 and v1.7.0. To overcome this issue in the latest versions I tried with the raw string actually it works fine.
_sql_template = Template(r'''
CREATE ROLE ${db_user}
LOGIN PASSWORD ${db_pass};

CREATE DATABASE ${db_name}
OWNER ${db_user}
ENCODING 'UTF8';

\connect ${db_name}

CREATE EXTENSION IF NOT EXISTS postgis CASCADE;
CREATE EXTENSION IF NOT EXISTS timescaledb CASCADE;
''')

Or we should've pinned the autopep v1.5.4 in lint.py.sh.
So can you please suggest an approach to get to the bottom of this issue?

@c0c0n3
Copy link
Member Author

c0c0n3 commented Jan 5, 2023

@Necravisaketi thanks for the analysis, that helps! I'd suggest

  • we pin the autopep version in lint.py.sh to be the same as in the github action
  • we future proof our code using raw strings as you pointed out

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants