Fix syntax error under Python 3.7 for async param #966

lopuhin · 2018-02-26T17:08:39Z

Syntax error under Python 3.7 fixed for twisted.conch.manhole and twisted.main.imap4. async_ keyword argument is used by default instead of async, but async is also supported for backwards compatibility.

Twisted issue: https://twistedmatrix.com/trac/ticket/9384

codecov · 2018-02-26T19:34:07Z

Codecov Report

Merging #966 into trunk will decrease coverage by 0.24%.
The diff coverage is 94.59%.

@@            Coverage Diff             @@
##            trunk     #966      +/-   ##
==========================================
- Coverage    91.9%   91.66%   -0.25%     
==========================================
  Files         844      843       -1     
  Lines      150622   149360    -1262     
  Branches    13143    13079      -64     
==========================================
- Hits       138436   136911    -1525     
- Misses      10098    10333     +235     
- Partials     2088     2116      +28

Accept async_ param instead of async, but also support passing async for backwards compatibility.

lopuhin · 2018-02-27T06:31:47Z

One build failed due to Invalid argument name "async_" (https://travis-ci.org/twisted/twisted/jobs/346467875#L537) - probably it does not like a trailing underscore. AFAIK async_ is a commonly used way to replace async argument, but I can change it to something like is_async as well - what would be better?

rodrigc · 2018-02-27T06:44:06Z

Try using _async for the variable and getAsyncParam() for the function. The https://github.com/twisted/twistedchecker program seems to accept that.

lopuhin · 2018-02-27T06:51:25Z

Try using _async for the variable and getAsyncParam() for the function.

Thanks @rodrigc , trying it in 0ad85ce

@rodrigc

Thanks @rodrigc for advice.

lopuhin · 2018-02-27T07:59:58Z

Thanks, looks like it fixed the build!

lopuhin · 2018-03-05T12:59:34Z

All scrapy tests pass under python 3.7 with this branch. Travis build passed at 30a298a, the last build failed due to an unrelated job erroring out. OS X build was stuck at "Waiting for status to be reported" both times. Is there anything that needs to be fixed?

rodrigc

Without your patch,
if I run the following command in the top level directory of the trunk branch in Twisted:

python -m tox -e pycodestyle

I get a few warnings like:

src/twisted/mail/imap4.py:1105:45: W606 'async' and 'await' are reserved keywords starting with Python 3.7

so your patch definitely fixes the problem.
This is very helpful, since this is a syntax error on Python 3.7.

adiroiban · 2018-03-23T09:43:18Z

src/twisted/conch/manhole.py

@@ -151,8 +153,9 @@ def _ebDisplayDeferred(self, failure, k, obj):
        return failure


-    def write(self, data, async=False):
-        self.handler.addOutput(data, async)
+    def write(self, data, _async=None, **kwargs):


instead of _async, can this be renamed to asAsync=True|False ? ...

+1, I like asAsync better than _async. I've seen async_ used in other libraries and it matches the PEP-8, but this name does not pass Twisted style check.

adiroiban · 2018-03-23T09:46:43Z

src/twisted/mail/imap4.py

-    def sendUntaggedResponse(self, message, async=False):
-        if not async or (self.blocked is None):
+    def sendUntaggedResponse(self, message, _async=None, **kwargs):
+        _async = get_async_param(_async, **kwargs)


shouldn't this trigger a deprecation warning?

I expect that in the future code should be updated to just use _async...

but since this is public API I am -1 for using _async as an argument name

and since this is used as a boolean flag, I am +1 for naming it like isAsync or some other name which implies that this is a boolean

adiroiban

Thanks @lopuhin for working on this and @rodrigc for the review :)

I am -1 on this due to the introduction if _async as a public API and it looks like @lopuhin agreed on doing the full transition and issue a deprecation warning.

adiroiban · 2018-03-23T09:59:56Z

The patch looks like fixing all the W606 'async' and 'await' are reserved keywords starting with Python 3.7 issue. I was running it as

$ python admin/pycodestyle-twisted.py --select=W606 src/twisted/

or with tox

$ tox -e pycodestyle '--select=W606 src/twisted/'

@adiroiban

As discussed with @adiroiban, this looks like a better choice because the API is public and it's a binary option

Recommend using isAsync instead

lopuhin · 2018-03-23T11:01:49Z

Thanks for review @adiroiban , I changed parameter name to isAsync in ed675f2 and added a deprecation warning in 1634587. Is it fine that the warning does not specify a twisted version when it's deprecated? If it's better to specify it, which version shall it use?

adiroiban · 2018-03-23T12:47:36Z

If it's better to specify it, which version shall it use?

I don't know what version to put in deprecations. @exarkun / @hawkowl / @glyph . Any suggestion?

I think that you cat put a placeholder like NEXT, and it will be automatically be updated... but I am not sure how this works and I don't see the documentation for it.

For deprecation, please use these helpers http://twistedmatrix.com/documents/current/api/twisted.python.deprecate.html

and the dev docs at http://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html#how-to-deprecate-apis

Please add docstrings to every test :)

adiroiban · 2018-03-23T12:49:43Z

src/twisted/python/compat.py

@@ -829,6 +830,34 @@ def _bytesRepr(bytestring):



+def get_async_param(isAsync=None, **kwargs):


I think that this should be private...as in, it should only be used inside Twisted core and not used by other projects... so this should be def _get_async_param

exarkun · 2018-03-23T12:50:33Z

I intended for the release management tooling to deal with the "NEXT" idea: https://twistedmatrix.com/trac/ticket/3266

That effort seems pretty well stalled. So I suppose it must be a manual release manager task, hopefully not undocumented but who knows. :/

adiroiban · 2018-03-23T12:50:59Z

src/twisted/test/test_compat.py

+        self.assertRaises(TypeError, get_async_param, False, {'async': False})
+
+
+    def test_get_async_param_deprecation(self):


The test should be for the public API itself, and not for this (what should be) private code.

adiroiban · 2018-03-23T12:52:23Z

src/twisted/newsfragments/9384.bugfix

@@ -0,0 +1,3 @@
+Syntax error under Python 3.7 fixed for twisted.conch.manhole and
+twisted.main.imap4. isAsync keyword argument is used by default instead of
+async, async keywoard argument is deprecated.


The deprecation info should be put into a separate file named src/twisted/newsfragments/9384.removal

In this way, the depreciation part of the release notes will contain the info about async keyword.
Otherwise, the bugfix part will contain both bugfix and deprecation.

rodrigc

This line is not being hit during the tests:

https://codecov.io/gh/twisted/twisted/compare/6a1d600241c273b693186f2a166b8e9c5717634e...93864a6674733b48e3264c1fe993858da0cd5c5f/diff#D6-856

Can you fix the code or fix your test so that this line is hit during the tests?

rodrigc · 2018-04-04T05:19:57Z

@lopuhin Can you fix the test?

lopuhin · 2018-04-04T07:46:52Z

@rodrigc sure, sorry that I stopped working on it - I plan to resume on Friday, but if someone wants to finish it sooner I'm totally fine with that too :)

hroncok · 2018-06-05T17:49:19Z

@lopuhin How can I help?

glyph · 2018-07-04T06:25:03Z

Replying to @exarkun from march:

That effort seems pretty well stalled. So I suppose it must be a manual release manager task, hopefully not undocumented but who knows. :/

https://github.com/twisted/incremental#updating

To do this within Twisted, simply write "Twisted NEXT".

hawkowl · 2018-07-16T08:16:38Z

Running tests on full builders. If it makes the tests run on 3.7 for imap/manhole, I'm happy.

lopuhin · 2018-07-20T06:02:55Z

Thanks a lot @hawkowl for finishing and merging the fix, sorry for letting it stall!

lopuhin mentioned this pull request Feb 26, 2018

Python 3.7 support scrapy/scrapy#3143

Closed

lopuhin force-pushed the 9384-remove-async-param branch from eb023b5 to decb928 Compare February 26, 2018 19:33

lopuhin added 2 commits February 26, 2018 23:17

Fix syntax error under Python 3.7 for async param

dcaf946

Accept async_ param instead of async, but also support passing async for backwards compatibility.

Add newsfragment

e669c72

lopuhin force-pushed the 9384-remove-async-param branch from decb928 to e669c72 Compare February 26, 2018 20:17

Use _async instead of async_ to fix quality build

30a298a

Thanks @rodrigc for advice.

lopuhin force-pushed the 9384-remove-async-param branch from 0ad85ce to 30a298a Compare February 27, 2018 07:31

Merge branch 'trunk' into 9384-remove-async-param

e1a8979

patiences mentioned this pull request Mar 4, 2018

[WIP] Run tests for Python 3.7 scrapy/scrapy#3150

Merged

Merge branch 'trunk' into 9384-remove-async-param

76eb35c

IlyaSkriblovsky mentioned this pull request Mar 15, 2018

9374 Fixing circular references in t.p.policies and t.p.tls #955

Merged

3 tasks

Merge branch 'trunk' into 9384-remove-async-param

b31a66f

rodrigc approved these changes Mar 23, 2018

View reviewed changes

Fix pycodestyle errors

ef0f83b

adiroiban reviewed Mar 23, 2018

View reviewed changes

adiroiban requested changes Mar 23, 2018

View reviewed changes

lopuhin added 2 commits March 23, 2018 13:58

Use isAsync name instead of _async

ed675f2

As discussed with @adiroiban, this looks like a better choice because the API is public and it's a binary option

Raise deprecation warning if async keyword is used

1634587

Recommend using isAsync instead

Fix line too long error

b8b680d

adiroiban reviewed Mar 23, 2018

View reviewed changes

lopuhin added 2 commits March 23, 2018 16:23

Make get_async_param private, add test docstrings

8784dee

Add a separate deprecation newsfragment

93864a6

rodrigc suggested changes Mar 23, 2018

View reviewed changes

lopuhin mentioned this pull request Jul 9, 2018

Python 3.7 support scrapy/scrapy#3326

Merged

Merge branch 'trunk' into 9384-remove-async-param

96fab7d

Fix mismerge

28fc5e5

hawkowl merged commit ee53504 into twisted:trunk Jul 16, 2018

lopuhin deleted the 9384-remove-async-param branch July 20, 2018 06:05

Laerte mentioned this pull request Jun 5, 2022

Drop Python 3.6 support scrapy/scrapy#5514

Merged

gudnimg mentioned this pull request Apr 20, 2024

#12130 Remove _get_async_param from twisted.python.compat #12132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix syntax error under Python 3.7 for async param #966

Fix syntax error under Python 3.7 for async param #966

lopuhin commented Feb 26, 2018

codecov bot commented Feb 26, 2018 •

edited

lopuhin commented Feb 27, 2018

rodrigc commented Feb 27, 2018 •

edited

lopuhin commented Feb 27, 2018

lopuhin commented Feb 27, 2018

lopuhin commented Mar 5, 2018

rodrigc left a comment

adiroiban Mar 23, 2018

lopuhin Mar 23, 2018

adiroiban Mar 23, 2018

adiroiban left a comment

adiroiban commented Mar 23, 2018

lopuhin commented Mar 23, 2018

adiroiban commented Mar 23, 2018

adiroiban Mar 23, 2018

exarkun commented Mar 23, 2018

adiroiban Mar 23, 2018

adiroiban Mar 23, 2018

rodrigc left a comment

rodrigc commented Apr 4, 2018 •

edited

lopuhin commented Apr 4, 2018

hroncok commented Jun 5, 2018

glyph commented Jul 4, 2018

hawkowl commented Jul 16, 2018

lopuhin commented Jul 20, 2018

		@@ -829,6 +830,34 @@ def _bytesRepr(bytestring):



		def get_async_param(isAsync=None, **kwargs):

		self.assertRaises(TypeError, get_async_param, False, {'async': False})


		def test_get_async_param_deprecation(self):

Fix syntax error under Python 3.7 for async param #966

Fix syntax error under Python 3.7 for async param #966

Conversation

lopuhin commented Feb 26, 2018

codecov bot commented Feb 26, 2018 • edited

Codecov Report

lopuhin commented Feb 27, 2018

rodrigc commented Feb 27, 2018 • edited

lopuhin commented Feb 27, 2018

lopuhin commented Feb 27, 2018

lopuhin commented Mar 5, 2018

rodrigc left a comment

Choose a reason for hiding this comment

adiroiban Mar 23, 2018

Choose a reason for hiding this comment

lopuhin Mar 23, 2018

Choose a reason for hiding this comment

adiroiban Mar 23, 2018

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban commented Mar 23, 2018

lopuhin commented Mar 23, 2018

adiroiban commented Mar 23, 2018

adiroiban Mar 23, 2018

Choose a reason for hiding this comment

exarkun commented Mar 23, 2018

adiroiban Mar 23, 2018

Choose a reason for hiding this comment

adiroiban Mar 23, 2018

Choose a reason for hiding this comment

rodrigc left a comment

Choose a reason for hiding this comment

rodrigc commented Apr 4, 2018 • edited

lopuhin commented Apr 4, 2018

hroncok commented Jun 5, 2018

glyph commented Jul 4, 2018

hawkowl commented Jul 16, 2018

lopuhin commented Jul 20, 2018

codecov bot commented Feb 26, 2018 •

edited

rodrigc commented Feb 27, 2018 •

edited

rodrigc commented Apr 4, 2018 •

edited