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

There is a problem with python 3.8 and escape sequence: #473

Closed
jtroussard opened this issue Jan 1, 2022 · 9 comments
Closed

There is a problem with python 3.8 and escape sequence: #473

jtroussard opened this issue Jan 1, 2022 · 9 comments

Comments

@jtroussard
Copy link
Contributor

There is a problem with python 3.8 and escape sequence:
'invalid escape sequence \*

________________________ ERROR collecting test session _________________________
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:383: in visit
    for x in Visitor(fil, rec, ignore, bf, sort).gen(self):
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:435: in gen
    for p in self.gen(subdir):
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:435: in gen
    for p in self.gen(subdir):
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:424: in gen
    dirs = self.optsort([p for p in entries
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:425: in <listcomp>
    if p.check(dir=1) and (rec is None or rec(p))])
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/main.py:667: in _recurse
    ihook = self.gethookproxy(dirpath)
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/main.py:482: in gethookproxy
    my_conftestmodules = pm._getconftestmodules(fspath)
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/config/__init__.py:431: in _getconftestmodules
    mod = self._importconftest(conftestpath.realpath())
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/config/__init__.py:470: in _importconftest
    raise ConftestImportFailure(conftestpath, sys.exc_info())
E   _pytest.config.ConftestImportFailure: (local('/conftest.py'), (<class 'SyntaxError'>, SyntaxError('invalid escape sequence \\*', ('/opt/virtualenvs/py38/lib/python3.8/site-packages/requests_oauthlib/oauth1_session.py', 258, 9, '        """Fetch a request token.\n')), <traceback object at 0x7fee844d6140>))

Originally posted by @matejsp in #443 (comment)

@hugovk
Copy link
Contributor

hugovk commented Jan 2, 2022

Cannot reproduce locally:

$ tox -e py38
GLOB sdist-make: /private/tmp/requests-oauthlib/setup.py
py38 inst-nodeps: /private/tmp/requests-oauthlib/.tox/.tmp/package/1/requests-oauthlib-1.3.0.zip
py38 installed: certifi==2021.10.8,cffi==1.15.0,charset-normalizer==2.0.9,coverage==5.5,coveralls==3.2.0,cryptography==3.4.8,docopt==0.6.2,idna==3.3,mock==4.0.3,oauthlib==3.1.1,pycparser==2.21,PyJWT==2.3.0,requests==2.26.0,requests-mock==1.9.3,requests-oauthlib @ file:///private/tmp/requests-oauthlib/.tox/.tmp/package/1/requests-oauthlib-1.3.0.zip,six==1.16.0,urllib3==1.26.7
py38 run-test-pre: PYTHONHASHSEED='1197210308'
py38 run-test: commands[0] | coverage run --source=requests_oauthlib -m unittest discover
....................................................................
----------------------------------------------------------------------
Ran 68 tests in 2.295s

OK
_________________________________________________________________ summary _________________________________________________________________
  py38: commands succeeded
  congratulations :)

Or on CI:

https://github.com/requests/requests-oauthlib/runs/4681605483?check_suite_focus=true

Is something else needed?

@matejsp
Copy link

matejsp commented Jan 2, 2022

Unfortunately Python is very nondeterministic.

In our case, this issue was triggered when collecting tests (some of tests importing requests-oauthlib) using pytest and PYTHONOPTIMIZE=1 with coverage enabled on our CI. When running application normally it didn't or locally on the machine this didn't happen.

I really don't know how to better reproduce this. However patching requests-oauthlib did help (\*\* -> ** -> no other changes). And \* is invalid escape sequence.

@matejsp
Copy link

matejsp commented Jan 2, 2022

Trying with sample test.py and current python that I use. It is not the same but similar behaviour.

test.py

def test_me():
    """ some \*\* """ 
    print("ok")

pytest test.py

(env39) ➜  ~ pytest test.py
=============================================================================================================== test session starts ================================================================================================================
platform darwin -- Python 3.9.9, pytest-4.6.11, py-1.10.0, pluggy-0.13.1
Using --random-order-bucket=module
Using --random-order-seed=93520

rootdir: /Users/user
plugins: freezegun-0.4.2, mock-1.11.2, metadata-1.10.0, random-order-0.8.1+bts1, celery-4.4.7, cov-2.11.1, django-3.10.0, html-1.22.1, forked-1.3.0, xdist-1.27.0
collected 1 item                                                                                                                                                                                                                                   

test.py .                                                                                                                                                                                                                                    [100%]

================================================================================================================= warnings summary =================================================================================================================
test.py:2
  /Users/user/test.py:2: DeprecationWarning: invalid escape sequence \*
    """ some \*\* """

-- Docs: https://docs.pytest.org/en/latest/warnings.html
======================================================================================================= 1 passed, 1 warnings in 0.06 seconds =======================================================================================================
(env39) ➜  ~ 

@hugovk
Copy link
Contributor

hugovk commented Jan 2, 2022

What file and line of requests-oauthlib is the problematic \*\*?

@matejsp
Copy link

matejsp commented Jan 2, 2022

requests_oauthlib/oauth1_session.py' line 258 (start of the docs lines)

Actual line is:
:param \*\*request_kwargs: Optional arguments passed to ''post''

https://github.com/requests/requests-oauthlib/blob/master/requests_oauthlib/oauth1_session.py#L270

@jtroussard
Copy link
Contributor Author

This doc string is flagged as a raw string, and that seems to be the fix for this warning. See stackoverflow link...

def fetch_request_token(self, url, realm=None, **request_kwargs):
        r"""Fetch a request token.

        This is the first step in the OAuth 1 workflow. A request token is
        obtained by making a signed post request to url. The token is then
        parsed from the application/x-www-form-urlencoded response and ready
        to be used to construct an authorization url.

        :param url: The request token endpoint URL.
        :param realm: A list of realms to request access to.
        :param \*\*request_kwargs: Optional arguments passed to ''post''
        ..."""

https://stackoverflow.com/questions/52335970/how-to-fix-string-deprecationwarning-invalid-escape-sequence-in-python

and more insight into this patch
https://bugs.python.org/issue27364

I think we are safe to say that this issue has been covered.

@hugovk
Copy link
Contributor

hugovk commented Jan 2, 2022

Thanks!

The r was added in #347 / 9d571a8 in December 2018 and released in v1.1.0.

If I delete the r and run the tests with pytest (which reports DeprecationWarnings by default), I see the error. Restoring the r fixes it.

Similarly, if I edit tox.ini to run tests with deprecation warnings turned on, I get the warning without the r and no warning with it:

-commands=coverage run --source=requests_oauthlib -m unittest discover
+commands=python -Wd -m coverage run --source=requests_oauthlib -m unittest discover

So I agree, this looks fixed.

@matejsp What version of requests-oauthlib are you using?

I would also suggest enabling deprecation warnings in tests, to help find and fix the relevant code is removed. Shall I create a PR with the above diff?

@matejsp
Copy link

matejsp commented Jan 2, 2022

We are using 1.0.0. That's the reason why we are missing "r" and getting this error. Seems to be fixed in later version but we are not able to upgrade yet. Sry for the false alarm.

Btw what should \*\* even represent? It is not valid escape sequence anyway (just in regex). Does a Sphinx or any other tool that needs this? Why not just using **kwargs ? star is not a reserved keyword (only in regex).

@jtroussard
Copy link
Contributor Author

jtroussard commented Jan 2, 2022

@matejsp It looks like it's a literal double asterisk to match the double asterisk in the signature, denoting an optional parameter, of course. Looks like this additional marking is breaking with the existing pattern, where the optional arguments are tagged with the double asterisk in the signature only. I think we can probably remove these, and the raw flag and close this issue.

Screen Shot 2022-01-02 at 11 23 49 AM

raised this PR #476 to level set this files documentation convention/pattern.

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

No branches or pull requests

3 participants