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

Already on GitHub? Sign in to your account

Fix display of plain text containing multiple carriage returns before line feed #2561

Merged
merged 1 commit into from Nov 12, 2012

Conversation

Projects
None yet
2 participants
Owner

takluyver commented Nov 8, 2012

Closes gh-2560

@ghost

ghost commented Nov 9, 2012

I've just been experimenting with this, and came up with some confusing results. After staring at that regex for about twenty minutes, I concluded that it'd only take an extra carriage return.

/^.*\r(?!\n)/gm becomes /^.*\r\r(?!\n)/gm

Whilst this fixed the adb output, it didn't work well when I tested it, so I ran the same test in the unmodified Notebook, and got confusing output there too.

This is from the modified version...

In [1]:
--------------------------------------
%%file test.py
print 'Unix \n'
print 'Mac \r'
print 'Windows \r\n'
print 'Google \r\r\n'
--------------------------------------
Overwriting test.py
--------------------------------------

In [2]:
--------------------------------------
run test
--------------------------------------
Unix 

Mac 
Windows 

Google

--------------------------------------
In [3]:
--------------------------------------
!python test.py
--------------------------------------
Unix 

Mac 

Windows 



--------------------------------------

I then repeated this in the unmodified Notebook...

In [1]:
--------------------------------------
%%file test.py
print 'Unix \n'
print 'Mac \r'
print 'Windows \r\n'
print 'Google \r\r\n'
--------------------------------------
Overwriting test.py
--------------------------------------
In [2]:
--------------------------------------
run test
--------------------------------------
Unix 

Mac 
Windows 


--------------------------------------
In [3]:
--------------------------------------
!python test.py
--------------------------------------
Unix





--------------------------------------

Maybe I'm totally missing something, but that seems all wrong to me. Whilst neither work, the modified version does seem to work under normal use, when it doesn't have to deal with crazy line endings.

Owner

takluyver commented Nov 9, 2012

Don't forget Python appends your system newline (presumably \n for both of us) after print statements. Put a comma at the end to suppress that: print 'Mac \r',. This works as I'd expect for me - try Ctrl-F5 to clear your browser cache?

Running commands using ! also seems to turn \n into \r\n - I think this is something pexpect is doing. My changes do account for that.

Here's what I see from your test script, both with %run and !python, on this branch:

Unix 

Mac 
Windows 

Google 

Because the print statement appends \n to each, they all get an extra newline, except the Mac \r, line which gets turned into a Windows line ending, \r\n. If I put commas after each print, I see:

Unix 
Windows 
Google 

Again, that's what I'd expect - the Mac line gets killed by the lone \r.

Owner

takluyver commented Nov 9, 2012

Oh, hang on, you were talking about your modified version. That would go wrong. Dissecting your regex: /^.*\r\r(?!\n)/gm:

  • Start of line ^
  • Any text .*
  • Two consecutive carriage returns \r\r
  • Not followed by a newline (?!\n)

My change (which I was testing), splits it into two regexes:

  • /\r+\n/gm - any number of carriage returns, followed by a newline (becomes a newline)
  • /^.*\r+/gm - start of line, any text, any number of carriage returns (line blanked)
Owner

takluyver commented Nov 9, 2012

(And I must withdraw my earlier criticism of adb - presumably it just hardcodes Windows line endings, and then it's pexpect which is transforming \n into \r\n, so \r\n becomes \r\r\n. It's a combination of two minor oddities, rather than one major one)

@ghost

ghost commented Nov 10, 2012

Thanks for killing this one Thomas. I never learnt regular expressions, so I appreciate you sorting this. Nice one.

Owner

takluyver commented Nov 10, 2012

@Carreau @ellisonbg - As I don't often do notebook stuff, I'd like someone who does to give this a once over before I merge it. It's a 2-line change, so it should be quick. Thanks!

Owner

Carreau commented Nov 11, 2012

haven't tried, but I don't see any reason not to merge it if it helps on android.

@Carreau Carreau added a commit that referenced this pull request Nov 12, 2012

@Carreau Carreau Merge pull request #2561 from takluyver/notebook-carriage-return
Fix display of plain text containing multiple carriage returns before line feed
a16e0e1

@Carreau Carreau merged commit a16e0e1 into ipython:master Nov 12, 2012

1 check passed

default The Travis build passed
Details

@minrk minrk added a commit that referenced this pull request Mar 5, 2013

@minrk minrk Backport PR #2561: Fix display of plain text containing multiple carr…
…iage returns before line feed


Closes gh-2560
adac07b

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

@Carreau Carreau Merge pull request #2561 from takluyver/notebook-carriage-return
Fix display of plain text containing multiple carriage returns before line feed
9203a3d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment