GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-143055: Implementation of PEP 798 by adqm · Pull Request #143056 · python/cpython Conversation
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 81e86b3 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F143056%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the plan is to document this in 3.15 "What's New" later?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@ZeroIntensity Thanks for the review!
I haven't drafted a blurb for "What's New" yet, but I'm happy to do so. I can do that in a separate PR later, or I don't mind adding it to this one if that's better. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Probably easiest to add the What's New entry here too. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Will try to make a pass over the grammar next week if @lysnikolaou doesn't beat me to it |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I had a look at the grammar. The changes look great in general. I left a couple of minor comments, but feel free to merge if the PR is not blocked otherwise.
Maybe @pablogsal wants to give it a look as well?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| if_expression[expr_ty]: | ||
| | a=disjunction 'if' b=disjunction 'else' c=expression { _PyAST_IfExp(b, a, c, EXTRA) } | ||
| | invalid_if_expression |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this alternative is never reached, since, in the second pass, it's encountered in expression before we get to if_expression and only in invalid rules otherwise that will overwrite the error either way.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look! Yep, I think you're right that we can't reach invalid_if_expression here, good catch. I'll push that change today (along with a what's new entry).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| | &'(' (genexp | tuple | group) | ||
| | &'[' (listcomp | list) | ||
| | &'{' (dictcomp | setcomp | dict | set) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changing the order here is probably only about error messages, right? If so, have you checked whether this affects parsing performance at all? To be clear, I'm okay to sacrifice a bit of performance for better error messages.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yep, the reordering here was purely for error reporting. I hadn't done any performance testing before, but I did a quick, unscientific test this morning, doing some quick timings on ast.parse for a few things.
As might be expected, compared to the place I branched from, it looks like this branch is, in general, a little bit faster on comprehensions and a little bit slower on literal lists. It's a little hard to say how this works out on the whole, but the time for parsing the full standard library was always within ±2% every time I ran it. I can look more thoroughly if we're concerned about this, but this little test at least suggests that things haven't blown up on average.
Sample results:
| Test | Reason | This Branch | Merge base | Change |
|---|---|---|---|---|
[] (1e5 times) | empty list | 0.5838 | 0.5539 | ~5% slower |
[x, y, z, a, b, c] (1e5 times) | well-formed list | 1.4051 | 1.3827 | ~2% slower |
[1, 2, 3, 4, 5, 6 (1e5 times) | broken list | 1.1750 | 1.1413 | ~3% slower |
[x for x in y if z and a] (1e5 times) | well-formed comprehension | 1.1642 | 1.2393 | ~6% faster |
[x for x in (1e5 times) | broken comprehension | 0.6568 | 0.7014 | ~6% faster |
[**x for x in y] (1e5 times) | broken comprehension unpacking (new specific error message) | 1.2675 | 1.0519 | ~20% slower |
[x if x else y] (1e5 times) | good ifexp | 1.0163 | 0.9890 | ~3% slower |
[*x if x else y] (1e5 times) | broken ifexp (new specific error message) | 1.3299 | 1.7870 | ~26% faster |
| full stdlib (5 times) | 26.1641 | 26.2642 | ~equal | |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Going to "Update branch" to see if it fixes CI |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @JelleZijlstra! Does "update branch" run a different set of tests? Unless I'm missing something, it looks like the test that's currently failing wasn't run on any of the earlier commits. Is that something I should be concerned about? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It's relatively new, so it wasn't on this PR before the update. I think it's an issue with |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Looks unrelated |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Exciting to see this merged! Thanks for the help, everyone :) If issues with the code do come up, I'm happy to help fix them. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Jelle Zijlstra <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.