◐ Shell
reader mode source ↗
Skip to content

bpo-30406: Make async and await proper keywords#1669

Merged
1st1 merged 26 commits into
python:masterfrom
JelleZijlstra:asyncsyntax
Oct 6, 2017
Merged

bpo-30406: Make async and await proper keywords#1669
1st1 merged 26 commits into
python:masterfrom
JelleZijlstra:asyncsyntax

Conversation

@JelleZijlstra

@JelleZijlstra JelleZijlstra commented May 19, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

AraHaan
AraHaan approved these changes May 19, 2017
View reviewed changes

@AraHaan AraHaan left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Looks alright to me.

@JelleZijlstra

JelleZijlstra commented May 19, 2017

Copy link
Copy Markdown
Member Author

I might need some time to fix the test failures.

@JelleZijlstra JelleZijlstra changed the title bpo-30406: Make async and await proper keywords May 19, 2017
@AraHaan

AraHaan commented May 19, 2017

Copy link
Copy Markdown
Contributor

Alright, But in general it looks like a nice start.

@JelleZijlstra JelleZijlstra changed the title bpo-30406: [WIP] Make async and await proper keywords May 19, 2017
@1st1 1st1 self-assigned this May 20, 2017
@vstinner vstinner requested a review from 1st1 May 20, 2017 05:38
@1st1

1st1 commented May 20, 2017

Copy link
Copy Markdown
Member

Overall looks good. I'll make a detailed review in a few days.

vstinner
vstinner reviewed May 20, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

https://docs.python.org/dev/reference/grammar.html should also be updated, no?

@JelleZijlstra

JelleZijlstra commented May 20, 2017

Copy link
Copy Markdown
Member Author

grammar.html just directly includes Grammar/Grammar, so it shouldn't require further changes.

@AraHaan

AraHaan commented May 20, 2017

Copy link
Copy Markdown
Contributor

@JelleZijlstra you mind also removing 'asyncio.async' as well as that thing might cause an SyntaxError.

@vstinner

vstinner commented May 20, 2017 via email

Copy link
Copy Markdown
Member

@JelleZijlstra

JelleZijlstra commented May 21, 2017

Copy link
Copy Markdown
Member Author

@AraHaan it's currently implemented as something like globals()['async'] = ensure_future, so the implementation won't cause a SyntaxError (as you can tell from the test suite passing). I suppose I could remove the alias, but I also don't want to make this patch bigger than necessary, so I'll wait for Yury to review it.

@AraHaan

AraHaan commented May 21, 2017

Copy link
Copy Markdown
Contributor

Ok

@1st1

1st1 commented Jun 10, 2017

Copy link
Copy Markdown
Member

I'll merge this when I return from my vacation in 11 days. Thank you for keeping the pr up to date!

@JelleZijlstra

JelleZijlstra commented Jul 20, 2017

Copy link
Copy Markdown
Member Author

@1st1 ping – have you had a chance to look at this PR?

1st1
1st1 reviewed Jul 20, 2017
View reviewed changes

@1st1 1st1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Overall looks good. Could you please rename it once again and add a news entry with blurb?

@1st1

1st1 commented Jul 20, 2017

Copy link
Copy Markdown
Member

s/rename/rebase

@JelleZijlstra

JelleZijlstra commented Jul 21, 2017

Copy link
Copy Markdown
Member Author

Just rebased from current master and added a news entry using blurb.

@JelleZijlstra

JelleZijlstra commented Jul 21, 2017

Copy link
Copy Markdown
Member Author

The AppVeyor failure is in test_idle and looks unrelated to this PR.

@JelleZijlstra

JelleZijlstra commented Jul 21, 2017

Copy link
Copy Markdown
Member Author

Oh, probably related to 7c5798e.

@vstinner

vstinner commented Jul 21, 2017

Copy link
Copy Markdown
Member

The AppVeyor failure is in test_idle and looks unrelated to this PR.

Rebase again and it should be fine.

1st1
1st1 requested changes Jul 21, 2017
View reviewed changes

@1st1 1st1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Playing with the code right now. One of the things you also need to do is to cleanup forbidden_name() function in Python/ast.c.

@1st1

1st1 commented Sep 26, 2017

Copy link
Copy Markdown
Member

Ping? I plan to work on this myself if this isn't resolved soon.

@JelleZijlstra

JelleZijlstra commented Sep 26, 2017

Copy link
Copy Markdown
Member Author

I was unable to resolve the issue I mentioned in #1669 (comment). I can push out the other fixes though and take another look within the next few days to see if I can get it working.

@1st1

1st1 commented Sep 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

I was unable to resolve the issue I mentioned in #1669 (comment). [..]

Interesting. I'll take a look at that too.

46 hidden items Load more…
@JelleZijlstra

JelleZijlstra commented Oct 6, 2017

Copy link
Copy Markdown
Member Author

Just rebased and adjusted the lib2to3 grammar changes to make local tests pass.

@1st1 1st1 merged commit ac31770 into python:master Oct 6, 2017
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Oct 9, 2017
@JelleZijlstra
bpo-31698: add REQ_NAME macro …
669d7a0 
@1st1 requested this in python#1669 (comment).
@JelleZijlstra JelleZijlstra mentioned this pull request Oct 9, 2017
Closed
@pablogsal pablogsal mentioned this pull request Oct 25, 2017
Merged
@llawall llawall mentioned this pull request Feb 19, 2018
Merged
@ambv

ambv commented Mar 12, 2018

Copy link
Copy Markdown
Contributor

@JelleZijlstra This change is incorrect in lib2to3. That library's grammar is explicitly kept in a way that is able to parse all code valid for Python 2 - Python 3.7. With your change we lost the ability to read valid Python 2 - Python 3.6 code that uses async and await as names.

@1st1

1st1 commented Mar 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Hm, I'm not sure what options we have here. Any suggestions? About the only one that I have is to undo the change for lib2to3 entirely (but keeping the rest of the changes).

@ambv

ambv commented Mar 12, 2018

Copy link
Copy Markdown
Contributor

Yes, this is what I'm inclined to do. Revert the lib2to3 part.

@JelleZijlstra

JelleZijlstra commented Mar 12, 2018

Copy link
Copy Markdown
Member Author

If I recall correctly I made changes to lib2to3 because some test was failing otherwise, so it might not be as simple as just reverting parts of this PR. I can spend some time looking into this later today.

@JelleZijlstra JelleZijlstra deleted the asyncsyntax branch March 12, 2018 21:03
@ambv ambv mentioned this pull request Mar 15, 2018
Closed
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Mar 18, 2018
@JelleZijlstra
Revert "bpo-30406: Make async and await proper keywords (python#1669)" …
5c138cf 
This reverts commit ac31770.

(Reverts only the lib2to3 part.)
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Mar 18, 2018
@JelleZijlstra
Revert "bpo-30406: Make async and await proper keywords (python#1669)" …
1f9ebe4 
This reverts commit ac31770.

(Reverts only the lib2to3 part.)
@JelleZijlstra JelleZijlstra mentioned this pull request Mar 18, 2018
Merged
@JelleZijlstra

JelleZijlstra commented Mar 18, 2018

Copy link
Copy Markdown
Member Author

OK, I made that up—tests pass cleanly if I undo my lib2to3 changes. @ambv @1st1 I submitted #6143 to fix the regression.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 18, 2018
@JelleZijlstra @miss-islington
Revert "bpo-30406: Make async and await proper keywords (pythonGH-1669)… …
e50f28e 
…" (pythonGH-6143)

This reverts commit ac31770.

(Reverts only the lib2to3 part.)
(cherry picked from commit f64aae4)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
ambv pushed a commit that referenced this pull request Mar 18, 2018
@JelleZijlstra @ambv
Revert "bpo-30406: Make async and await proper keywords (#1669)" (GH-… …
f64aae4 
…6143)

This reverts commit ac31770.

(Reverts only the lib2to3 part.)
miss-islington added a commit that referenced this pull request Mar 18, 2018
@miss-islington @JelleZijlstra
Revert "bpo-30406: Make async and await proper keywords (GH-1669)" (G… …
a90df50 
…H-6143)

This reverts commit ac31770.

(Reverts only the lib2to3 part.)
(cherry picked from commit f64aae4)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
jo2y pushed a commit to jo2y/cpython that referenced this pull request Mar 23, 2018
@JelleZijlstra @jo2y
Revert "bpo-30406: Make async and await proper keywords (python#1669)" ( …
314ac54 
pythonGH-6143)

This reverts commit ac31770.

(Reverts only the lib2to3 part.)
@ddfisher ddfisher mentioned this pull request Jun 30, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@1st1 1st1 1st1 approved these changes

+1 more reviewer

@AraHaan AraHaan AraHaan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@1st1 1st1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@JelleZijlstra @AraHaan @1st1 @vstinner @ambv @the-knights-who-say-ni @bedevere-bot