E52B bpo-38673: dont switch to ps2 if the line starts with comment or whitespace by isidentical · Pull Request #17421 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-38673: dont switch to ps2 if the line starts with comment or whitespace#17421

Merged
miss-islington merged 6 commits intopython:masterfrom
isidentical:bpo-38673
Dec 9, 2019
Merged

bpo-38673: dont switch to ps2 if the line starts with comment or whitespace#17421
miss-islington merged 6 commits intopython:masterfrom
isidentical:bpo-38673

Conversation

@isidentical
Copy link
Member
@isidentical isidentical commented Nov 30, 2019 •
edited by miss-islington
Loading

@terryjreedy
Copy link
Member
terryjreedy commented Dec 1, 2019

The patch fixes the issue on master on Windows. I can't tell if this is the best fix or if there could be an issue not caught in the tests.

gvanrossum
gvanrossum reviewed Dec 1, 2019
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I'm not sure that this is the right approach yet. While you prevent the prompt from changing to "...", the code still keeps accumulating input, which means the line count will be off. Example:

>>> #
>>> )
  File "<stdin>", line 2
SyntaxError: unmatched ')'

I suspect you may have to doctor the caller of tok_nextc(), or perhaps its caller...

@isidentical
Copy link
Member Author
isidentical commented Dec 2, 2019< 10BC0 /a>

Thanks for suggestions @gvanrossum, current version fixed accumulation bug.

pablogsal
pablogsal reviewed Dec 2, 2019
View reviewed changes
Parser/tokenizer.c Outdated
if (tok->nextprompt != NULL)
tok->prompt = tok->nextprompt;
if (tok->nextprompt != NULL) {
if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))
Copy link
Member
@pablogsal pablogsal Dec 2, 2019

Choose a reason for hiding this comment

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

PEP 7: Please add braces

pablogsal
pablogsal reviewed Dec 2, 2019
View reviewed changes
Parser/tokenizer.c Outdated
if (tok->nextprompt != NULL) {
if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))
tok->nextprompt = NULL;
else
Copy link
Member
@pablogsal pablogsal Dec 2, 2019

Choose a reason for hiding this comment

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

PEP 7: Please add braces

pablogsal
pablogsal reviewed Dec 2, 2019
View reviewed changes
Parser/tokenizer.c Outdated
if (tok->nextprompt != NULL)
tok->prompt = tok->nextprompt;
if (tok->nextprompt != NULL) {
if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))
Copy link
Member
@pablogsal pablogsal Dec 2, 2019

Choose a reason for hiding this comment

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

Suggestion: You may be able to simplify this with strchr (check that is not slower....maybe is faster if inlined).

Copy link
Member Author
@isidentical isidentical Dec 3, 2019

Choose a reason for hiding this comment

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

Thank you for suggestions, I'll apply them.

@pablogsal
Copy link
Member
pablogsal commented Dec 2, 2019

Left some comment regarding the code itself, I didn't check the solution itself yet.

@gvanrossum
Copy link
Member
gvanrossum commented Dec 2, 2019

(I am working through a backlog, I should pay attention to reviews and other tasks that have been waiting longer first.)

gvanrossum
gvanrossum reviewed Dec 6, 2019
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Better, but there's still a regression. Look here:

Python 3.9.0a1+ (heads/pr/17421:5e632fb43a, Dec  6 2019, 14:23:30) 
[Clang 11.0.0 (clang-1100.0.33.8)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> if 1:
...   # comment
  File "<stdin>", line 2
    # comment
    ^
IndentationError: expected an indented block
>>>

Previously this would just give a ... prompt:

Python 3.8.0 (v3.8.0:fa919fdf25, Oct 14 2019, 10:23:27) 
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> if 1:
...   # comment
...   pass
... 
>>>

@isidentical
Copy link
Member Author
isidentical commented Dec 7, 2019

I think it works now but i am not sure if it is an ideal solution or not.

gvanrossum
gvanrossum reviewed Dec 7, 2019
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I have to think about this more. It seems to work but the changes look ad-hoc (and not at all what I had expected they would be when I wrote up the bug description). I need to do some deep thinking about whether this is the right place in the code to implement this behavior.

Ping me if I haven't replied in a week -- I don't want to keep you hanging forever!

@gvanrossum
Copy link
Member
gvanrossum commented Dec 8, 2019

I have a different suggestion:

--- a/Parser/tokenizer.c
+++ b/Parser/tokenizer.c
@@ -1148,6 +1148,12 @@ tok_get(struct tok_state *tok, char **p_start, char **p_end)
             if (col == 0 && c == '\n' && tok->prompt != NULL) {
                 blankline = 0; /* Let it through */
             }
+            else if (tok->prompt != NULL && tok->lineno == 1) {
+                /* In interactive mode, if the first line contains
+                   only spaces and a comment, let it through. */
+                blankline = 0;
+                col = altcol = 0;
+            }
             else {
                 blankline = 1; /* Ignore completely */

@isidentical
Copy link
Member Author
isidentical commented Dec 8, 2019

Thanks for your suggestions Guido.

gvanrossum
gvanrossum approved these changes Dec 8, 2019
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM. Can one of the other reviewers double-check there's nothing amiss?

@terryjreedy
Copy link
Member
terryjreedy commented Dec 9, 2019

The 2 error cases on the issue and 2 above and otherw work for me. I presume

if 1:
... # k
... # k
... a = 2
... )
File "", line 5
SyntaxError: unmatched ')'

is correct.

@miss-islington miss-islington merged commit 109fc27 into python:master Dec 9, 2019
@miss-islington
Copy link
Contributor
miss-islington commented Dec 9, 2019

Thanks @isidentical for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
@isidentical @miss-islington
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
b5c2992 
…espace (pythonGH-17421)

https://bugs.python.org/issue38673
(cherry picked from commit 109fc27)

Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>
@bedevere-bot
Copy link
bedevere-bot commented Dec 9, 2019

GH-17516 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Dec 9, 2019
@miss-islington @isidentical
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
184a381 
…espace (GH-17421)

https://bugs.python.org/issue38673
(cherry picked from commit 109fc27)

Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
@miss-islington
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
0ef5a7f 
…espace (pythonGH-17421)

https://bugs.python.org/issue38673
(cherry picked from commit 109fc27)

Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>
(cherry picked from commit 184a381)

Co-authored-by: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>
ned-deily pushed a commit that referenced this pull request Dec 9, 2019
@miss-islington @isidentical @ned-deily
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
188d5ae 
…espace (GH-17421) (GH-17522)

https://bugs.python.org/issue38673
(cherry picked from commit 109fc27)

Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@isidentical @shihai1991
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
1119d98 
…espace (pythonGH-17421)

https://bugs.python.org/issue38673
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@isidentical @terryjreedy @pablogsal @gvanrossum @miss-islington @bedevere-bot @the-knights-who-say-ni
0