Skip to content

Conversation

ambv
Copy link
Contributor

@ambv ambv commented May 22, 2017

This partially solves bpo-23894. A separate PR coming later to support f-strings.

@ambv ambv added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels May 22, 2017
@ambv ambv requested a review from ned-deily May 22, 2017 18:46
@ambv ambv changed the title Make rb'' strings work in lib2to3 bpo-23894: Make rb'' strings work in lib2to3 May 22, 2017
@ambv ambv force-pushed the bpo-23894-rb branch 2 times, most recently from 76b0e9d to d330004 Compare May 22, 2017 19:04
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"String" didn't previously include b but this is suspicious to me. Why wouldn't it? That way b'' literals were only PseudoTokens (via ContStr), not actual Tokens (via String).

Copy link
Member

Choose a reason for hiding this comment

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

LG

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test: LGTM. As far as the tokenizer changes, I'm not familiar enough with lib2to3 to provide a useful review. Assuming other reviewers have no objections, I'm fine for this going into 3.6.2rc.

@ambv
Copy link
Contributor Author

ambv commented May 22, 2017

Tests are failing because I used an f-string in the test and lib2to3 is parsing that test file as part of fixers. f-strings are not supported yet in lib2to3. Switching to using a classic str.format fixes it. I'm updating the PR now.

UPDATE: as expected, tests pass now.

This partially solves bpo-23894.
@ambv ambv requested review from vstinner and gvanrossum May 22, 2017 19:42
@ambv ambv merged commit 0c4aca5 into python:master May 22, 2017
ambv added a commit to ambv/cpython that referenced this pull request May 22, 2017
This partially solves bpo-23894.
(cherry picked from commit 0c4aca5)
ambv added a commit that referenced this pull request May 22, 2017
This partially solves bpo-23894.
(cherry picked from commit 0c4aca5)
@vstinner
Copy link
Member

I was requested for a review, but after I completed my review, I see that the change was already merged :-D It's fine, don't worry. I just want to give my post-commit LGTM! Nice change. Thanks @ambv for taking care of 2to3 ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants