Fixes and improvements to the input splitter #2110

Merged
merged 4 commits into from Jul 8, 2012

Conversation

Projects
None yet
4 participants
Contributor

asmeurer commented Jul 8, 2012

This fixes #2108 (see the commit message for more details), and also now yield, break, and continue will automatically deindent (a deficiency I noticed when I was editing the file). Unless I'm mistaken, those keywords all end blocks.

This also clears a lot of trailing whitespace in the files I've edited, as I have my editor set to do so. I hope you don't mind.

I've also added an unrelated fix to the README that I noticed as well, but is too small to put in a separate PR.

I believe the tests pass, but I'm pretty sure you have test failures in master, and your test runner's output is quite confusing to me, so please review carefully (also because this is a core part of the code and because I am a new contributor).

asmeurer added some commits Jul 8, 2012

@asmeurer asmeurer Line continuations now terminate after one blank line (#2108)
Previously, we had

In [1]: 1\
...:
...:
...:
...:

In other words, no amount of blank lines would terminate after a line
continuation, in contrast with regular Python:

>>> 1\
...
1

This made things really annoying when I typed \ instead of a newline--quite
easy to do since they are right next to each other on the keyboard.

Now, we have

In [1]: 1\
...:
Out[1]: 1

This also fixes another related behavioral difference between IPython.  If a
space follows a line continuation character, it should be a
SyntaxError("unexpected character after line continuation character"), even if
the line is otherwise continuable, according to regular Python (e.g., `1 \ `
or `(1 + \ `).  This now consistent between the two.

Closes #2108
9bd0c90
@asmeurer asmeurer `yield`, `break`, and `continue` automatically dedent
Previously `return`, `raise`, and `pass` did; now these do as well.  The tests
for these were copied from the tests for those.
df408e2
Contributor

asmeurer commented Jul 8, 2012

Test results for commit 2783236 merged into master
Platform: linux2

  • python3.2: OK (libraries not available: cython matplotlib numpy oct2py pygments pymongo qt rpy2 tornado wx wx.aui zmq)

Not available for testing: python2.6, python2.7, python3.1

Contributor

asmeurer commented Jul 8, 2012

Oh, and I'll repeat the question from #2108 here: should I add any additional tests that 1 \ and (1 \ (with a space at the end) raise SyntaxError("unexpected character after line continuation character")? I don't understand enough about how IPython executes code to know if that's something that needs to be ensured on the IPython side, or if the way you do it, Python itself will always ensure that it happens.

Contributor

asmeurer commented Jul 8, 2012

Is test_pr supposed to actually run the tests? I don't think it did.

Owner

fperez commented Jul 8, 2012

Yes, test_pr runs the tests. For some reason in your case it only found the 3.2 installation, not sure why. Running it now.

Owner

fperez commented Jul 8, 2012

the 1\ case is not quite right in ipython, in the sense that we continue taking user input, and only raise the correct SyntaxError after evaluating the whole block. We should be stopping early in that case and bailing immediately, like we do with other types of invalid input (try ajlfasdj l;asj lfasdj f;lasdj-type input, and it does bail right away).

If you feel like digging into the inputsplitter code deeper to fix that, great. But if not, no worries, please just leave an open issue for it so we can return to it later. It's not like you've introduced a new bug here, so that wouldn't matter for this PR.

@fperez fperez commented on the diff Jul 8, 2012

IPython/core/inputsplitter.py
@@ -202,17 +206,17 @@ def remove_comments(src):
"""
return re.sub('#.*', '', src)
-
@fperez

fperez Jul 8, 2012

Owner

In general we prefer not to have too much of these whitespace-only changes because they make the diffs longer to read, and we don't have a super-strict whitespace policy, so different contributors have their editors set differently.

In this case there's not that much to be a bother, as fortunately this PR only touches few files, so we'll let it pass. But if you ever make a PR that touches a ton of file, please refrain from adding whitespace-only changes all over, as it will make review harder.

@asmeurer

asmeurer Jul 8, 2012

Contributor

Sorry about that.

@fperez

fperez Jul 8, 2012

Owner

no worries, no biggie. Just a tip for the future.

Owner

fperez commented Jul 8, 2012

Test results for commit b4c0a20 merged into master
Platform: linux2

  • python2.7: Failed, log at https://gist.github.com/3069587
  • python3.2: OK (libraries not available: cython matplotlib oct2py pygments pymongo rpy2 wx wx.aui)

Not available for testing: python2.6, python3.1

Contributor

asmeurer commented Jul 8, 2012

My Python installs are somewhat messed up, so I'm not surprised it doesn't work.

Contributor

asmeurer commented Jul 8, 2012

Can be more clear what you mean? If I type 1 \ or (1 \, followed by a newline, it gives a SyntaxError immediately.

Owner

fperez commented Jul 8, 2012

Test results for commit b4c0a20 merged into master
Platform: linux2

  • python2.7: OK
  • python3.2: OK (libraries not available: cython matplotlib oct2py pygments pymongo rpy2 wx wx.aui)

Not available for testing: python2.6, python3.1

Owner

fperez commented Jul 8, 2012

Mmh, in trunk in IPython, it lets me continue entering code:

In [2]: 1 \ 
   ...: 
   ...: 3
  File "<ipython-input-2-835572ca0d52>", line 1
    1 \
        ^
SyntaxError: unexpected character after line continuation character

whereas plain python does this:

>>> 1 \ 
  File "<stdin>", line 1
    1 \ 
       ^
SyntaxError: unexpected character after line continuation character

raising the error immediately and without letting me continue.

That's the part we're not doing right in ipython, we should stop taking input immediately.

Owner

fperez commented Jul 8, 2012

The rest of the PR looks good, so just let me know if you want to tackle the \ case here or leave it for another day, case in which I'll merge.

Contributor

asmeurer commented Jul 8, 2012

I get (in this branch of course):


In [1]: 1 \
   ...: 
Out[1]: 1

In [2]: 1 \ 
  File "<ipython-input-2-6486e5ccd025>", line 1
    1 \
        ^
SyntaxError: unexpected character after line continuation character

(the second one has a space after the \)

Owner

fperez commented Jul 8, 2012

Ah, OK, then it also fixes that, I hadn't realized. Great!

Contributor

asmeurer commented Jul 8, 2012

Yes, before this PR, \\n and \ \n were both treated the same because of the rstrip(). So the unexpected character SyntaxError is new to IPython (hence why I'm wondering if and how it should be tested).

Owner

fperez commented Jul 8, 2012

OK, then the way to test this is to call ip.run_cell('1 \ ') and check that it raises a SyntaxError. Then we'll have a test for that as well, and we can merge this puppy. Great job, thanks!

Contributor

asmeurer commented Jul 8, 2012

Where should the test go?

Owner

fperez commented Jul 8, 2012

I'd add it to the main interactiveshell tests, since it calls a method of the main object.

Contributor

asmeurer commented Jul 8, 2012

Sorry, but how do I test that an exception was raised? I didn't see any other tests that did it. self.assertRaises(SyntaxError) doesn't seem to work.

Owner

fperez commented Jul 8, 2012

Mmh, funny: the codepath for asserRaises misses syntax errors. Oh well, do it the old-fashioned way:

def test_syntax_error():
    try:
        ip.run_cell('a b')
    except SyntaxError:
        pass
Contributor

asmeurer commented Jul 8, 2012

OK, this took quite a bit of debugging of what appeared to be quite strange behavior before I figured it out. I don't think that run_cell raises an exception in the case of SyntaxError.

Consider for example:

In [1]: ip = get_ipython()

In [2]: try:
   ...:     ip.run_cell("a b")
   ...:     print 1
   ...: except:
   ...:     print 2
   ...: 
  File "<ipython-input-2-7557d2f3a6ad>", line 1
    a b
      ^
SyntaxError: invalid syntax

1

So I'd say that's definitely a bug, no?

I need to go to bed. I'll look at this again tomorrow.

Contributor

asmeurer commented Jul 8, 2012

Ah, maybe it isn't a bug, but intended behavior. I thought it wasn't doing that for other exceptions, but I guess it is.

So the question then is, how exactly do I test this? Setup an exception hook before running the cell perhaps?

Contributor

rkern commented Jul 8, 2012

yield does not end a block. It should not deindent.

Contributor

asmeurer commented Jul 8, 2012

Dang. I was afraid that might be the case, as I use that particular feature of Python quite rarely. I guess I see now how this is so. break and continue should be good, though, right?

Contributor

rkern commented Jul 8, 2012

Yes break and continue are good additions.

Contributor

asmeurer commented Jul 8, 2012

OK, yield dedent removed.

Owner

fperez commented Jul 8, 2012

I see, our syntaxerror handling is so deeply woven into the execution code that it's nearly impossible to test this from the outside. And I also can't seem to make doctest handle a syntax error.

At this point, I think it's just not worth it: we can't sink days into this. This one will just have to go in without a test (other than your manual verification). I think this is otherwise good to go, anything else you have in mind?

@rkern, thanks for the yield catch!

Contributor

asmeurer commented Jul 8, 2012

I guess that's fine. As long as you don't readd an rstrip, it should stay this way.

Owner

fperez commented Jul 8, 2012

OK, merging now. Thanks!

@fperez fperez added a commit that referenced this pull request Jul 8, 2012

@fperez fperez Merge pull request #2110 from asmeurer/inputsplitter
Fixes and improvements to the input splitter

This fixes #2108 (see the commit message for more details), and also now break, and continue will automatically deindent.
c7401c9

@fperez fperez merged commit c7401c9 into ipython:master Jul 8, 2012

Owner

takluyver commented Jul 13, 2012

FWIW, I think the way to check if a syntax error occurs via run_cell() is to mock the showsyntaxerror() method, then assert that it got called.

Owner

fperez commented Jul 20, 2012

Good catch, @takluyver; I didn't think of doing that :)

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

@fperez fperez Merge pull request #2110 from asmeurer/inputsplitter
Fixes and improvements to the input splitter

This fixes #2108 (see the commit message for more details), and also now break, and continue will automatically deindent.
a9e1ba4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment