Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes 11334/11354 Multi line code blocks #11358

Merged
merged 17 commits into from Oct 8, 2018
Merged

Fixes 11334/11354 Multi line code blocks #11358

merged 17 commits into from Oct 8, 2018

Conversation

@tonyfast
Copy link
Contributor

@tonyfast tonyfast commented Oct 4, 2018

This pull request merges the recent input transformer changes from master. It builds off of the work by @bartskowron in #11354 to resolve #11334 . The changes pass tests locally, including the new tests provided by @Carreau; let's see what CI says.

@takluyver I commented out an assertion. I am not sure what the statement, is this change ok?

bxsx and others added 8 commits Oct 1, 2018
When codeop.compile_command() returns None it actually says "at least
some part of the code was compiled successfully" which is not really important
for checking if it's complete or not.
Once we haven't got any errors during compilation process, we just want
to check if there will be another nested block of code or not by checking
a colon.
An assert statement needed to be commented out.  I'm not sure what the statement was stating though.
@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 4, 2018

Anyone have any ideas why this may be passing on everything, but Python 3.7dev?

@Carreau
Copy link
Member

@Carreau Carreau commented Oct 4, 2018

Anyone have any ideas why this may be passing on everything, but Python 3.7dev?

Maybe a change of API in generate_token or similar on Pyhton 3.7 dev.

@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 4, 2018

I'll scope it out. Thanks.

@Carreau
Copy link
Member

@Carreau Carreau commented Oct 4, 2018

@Carreau
Copy link
Member

@Carreau Carreau commented Oct 4, 2018

## 3.6
In [8]: list(generate_tokens(iter('a = 1').__next__))
Out[8]:
[TokenInfo(type=1 (NAME), string='a', start=(1, 0), end=(1, 1), line='a'),
 TokenInfo(type=0 (ENDMARKER), string='', start=(2, 0), end=(2, 0), line='')]

AFAICT we need need to add a \n (NEWLINE) before ENDMARKER

@Carreau Carreau requested a review from takluyver Oct 4, 2018
These changes are necessary for a failure on Python 3.7 dev https://bugs.python.org/issue33899
@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 4, 2018

Thanks for these references @Carreau . I added some conditions for the newline tokens. Let's see if this helps.

@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 4, 2018

The CPython changes to tokenize effect 3.6 & 3.7. The IPython tests are not checking on Python3.6dev.
Should there be tests for 3.6dev because of this use case?

I'm currently working on this pr on py37rc1.

@Carreau
Copy link
Member

@Carreau Carreau commented Oct 4, 2018

@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 4, 2018

If we consider a simple statement like a=999.

In Python 3.6 with the old tokenize:

Version info sys.version_info(major=3, minor=6, micro=6, releaselevel='final', serial=0)
[TokenInfo(type=1 (NAME), string='a', start=(1, 0), end=(1, 1), line='a=999'),
 TokenInfo(type=53 (OP), string='=', start=(1, 1), end=(1, 2), line='a=999'),
 TokenInfo(type=2 (NUMBER), string='999', start=(1, 2), end=(1, 5), line='a=999'),
 TokenInfo(type=0 (ENDMARKER), string='', start=(2, 0), end=(2, 0), line='')]

compared to in Python 3.7 on release candidate.

Version info sys.version_info(major=3, minor=7, micro=1, releaselevel='candidate', serial=1)
[TokenInfo(type=1 (NAME), string='a', start=(1, 0), end=(1, 1), line='a=999'),
 TokenInfo(type=53 (OP), string='=', start=(1, 1), end=(1, 2), line='a=999'),
 TokenInfo(type=2 (NUMBER), string='999', start=(1, 2), end=(1, 5), line='a=999'),
 TokenInfo(type=4 (NEWLINE), string='', start=(1, 5), end=(1, 6), line=''),
 TokenInfo(type=0 (ENDMARKER), string='', start=(2, 0), end=(2, 0), line='')]

adding a newline to the end of the statement will created a consistent tokenization.

Version info sys.version_info(major=3, minor=6, micro=6, releaselevel='final', serial=0)
[TokenInfo(type=1 (NAME), string='a', start=(1, 0), end=(1, 1), line='a=999\n'),
 TokenInfo(type=53 (OP), string='=', start=(1, 1), end=(1, 2), line='a=999\n'),
 TokenInfo(type=2 (NUMBER), string='999', start=(1, 2), end=(1, 5), line='a=999\n'),
 TokenInfo(type=4 (NEWLINE), string='\n', start=(1, 5), end=(1, 6), line='a=999\n'),
 TokenInfo(type=0 (ENDMARKER), string='', start=(2, 0), end=(2, 0), line='')]
@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 5, 2018

@Carreau it looks like the core tests are passing, but I am receiving a timeout error in the terminal test group . Do you have an insight into this?

Co-Authored-By: Nicholas Bollweg <bollwyvl@users.noreply.github.com>
@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 6, 2018

The errors at the moment are in SystemAssign. The test case =! is trying to assign a system statement.

tonyfast and others added 3 commits Oct 6, 2018
Co-Authored-By: Nicholas Bollweg <bollwyvl@users.noreply.github.com>
Co-Authored-By: Nicholas Bollweg <bollwyvl@users.noreply.github.com>
Co-Authored-By: Nicholas Bollweg <bollwyvl@users.noreply.github.com>
@tonyfast
Copy link
Contributor Author

@tonyfast tonyfast commented Oct 6, 2018

Woo! The new and old tests are passing now. These fixes resolve #11334 #11354 #11346 #11337 .

@Carreau @takluyver could you review this when you get a chance?

@Carreau
Copy link
Member

@Carreau Carreau commented Oct 6, 2018

Thanks ! Awesome work @tonyfast !

I'll try to review and merced this week-end.

@@ -34,6 +34,7 @@ after_success:

matrix:
include:
- { python: "3.6-dev", dist: xenial, sudo: true }

This comment has been minimized.

@Carreau

Carreau Oct 6, 2018
Member

I'm just going to add some "ok", to follow where I am in the review for myself.

This comment has been minimized.

@@ -234,6 +234,7 @@ def find(cls, tokens_by_line):
for line in tokens_by_line:
assign_ix = _find_assign_op(line)
if (assign_ix is not None) \
and not line[assign_ix].line.strip().startswith('=') \

This comment has been minimized.

@@ -369,11 +370,15 @@ def transform(self, lines):
end_line = find_end_of_continued_line(lines, start_line)
line = assemble_continued_line(lines, (start_line, start_col), end_line)

if line[:2] in ESCAPE_DOUBLES:
if len(line) > 1 and line[:2] in ESCAPE_DOUBLES:

This comment has been minimized.

if escape in tr:
call = tr[escape](content)
else:
call = ''

This comment has been minimized.

# Append an newline for consistent tokenization
# See https://bugs.python.org/issue33899
cell += '\n'

This comment has been minimized.

@Carreau

Carreau Oct 6, 2018
Member

Seem good. Looks like we should either ends with 2 \n or None.

if not lines:
return 'complete', None

if lines[-1].endswith('\\'):

This comment has been minimized.

@@ -212,6 +214,17 @@ def test_check_complete():
nt.assert_equal(cc("a = '''\n hi"), ('incomplete', 3))
nt.assert_equal(cc("def a():\n x=1\n global x"), ('invalid', None))
nt.assert_equal(cc("a \\ "), ('invalid', None)) # Nothing allowed after backslash
nt.assert_equal(cc("1\\\n+2"), ('complete', None))
nt.assert_equal(cc("1\\\n+2"), ('complete', None))

This comment has been minimized.

@bxsx

bxsx Oct 6, 2018
Contributor

redundant test

Nice catch @bartskowron
@Carreau Carreau added this to the 7.1 milestone Oct 8, 2018
@Carreau
Copy link
Member

@Carreau Carreau commented Oct 8, 2018

Thanks all, let's try to get that in and have an IPython 7.1 by end of week.
That should match the imminent release of Python 3.7.1 that have the compatibility issues mentioned earlier.

@Carreau Carreau merged commit 8e29b01 into ipython:master Oct 8, 2018
4 checks passed
4 checks passed
codecov/patch 92.85% of diff hit (target 0%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +24.63% compared to b3edf6c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants