Fix for prompts containing newlines. #1105

Merged
merged 2 commits into from Dec 8, 2011

Conversation

Projects
None yet
4 participants
@takluyver
Member

takluyver commented Dec 5, 2011

@rkern, I've tried this with the config example you supplied, and it seems to be working OK.

Specifically, we now only justify prompts based on the last line of the preceding prompt, and we don't attempt to justify multiline prompts at all.

Closes gh-1104

@rkern

This comment has been minimized.

Show comment
Hide comment
@rkern

rkern Dec 5, 2011

Contributor

Works for me.

Contributor

rkern commented Dec 5, 2011

Works for me.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 5, 2011

Member

Great, thanks. I'll give the others a day or two to make any comments, then merge it.

Member

takluyver commented Dec 5, 2011

Great, thanks. I'll give the others a day or two to make any comments, then merge it.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 6, 2011

Member

works fine for me, thanks.

Member

minrk commented Dec 6, 2011

works fine for me, thanks.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 6, 2011

Member

Glad to hear the fixes work correctly. I thought for a bit about what kind of automated test we could add to ensure this doesn't return to bite us later, but I can't quite seem to see a quick way to make one. I figured we could do one involving creating an irunner with custom prompts, running a session through it and validating back the output, but it sounds like a fair amount of hassle and I'm not sure the effort/payoff is worth it.

If you can think of a way to test this that isn't too painful to implement, go ahead and do it, otherwise merge as-is.

Member

fperez commented Dec 6, 2011

Glad to hear the fixes work correctly. I thought for a bit about what kind of automated test we could add to ensure this doesn't return to bite us later, but I can't quite seem to see a quick way to make one. I figured we could do one involving creating an irunner with custom prompts, running a session through it and validating back the output, but it sounds like a fair amount of hassle and I'm not sure the effort/payoff is worth it.

If you can think of a way to test this that isn't too painful to implement, go ahead and do it, otherwise merge as-is.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 6, 2011

Member

I think we could do a bit of unit testing with the .width attribute,
setting different prompts. But I'm not going to implement it before
sleeping. ;-)

Member

takluyver commented Dec 6, 2011

I think we could do a bit of unit testing with the .width attribute,
setting different prompts. But I'm not going to implement it before
sleeping. ;-)

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 6, 2011

Member

Sure :) We've made good progress on the other PRs today, so we can sit on this one for a few days before the 0.12 rc.

Member

fperez commented Dec 6, 2011

Sure :) We've made good progress on the other PRs today, so we can sit on this one for a few days before the 0.12 rc.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 8, 2011

Member

I've added a few simple tests, and checked that they're passing.

Member

takluyver commented Dec 8, 2011

I've added a few simple tests, and checked that they're passing.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 8, 2011

Member

nice, looks good to me.

Member

minrk commented Dec 8, 2011

nice, looks good to me.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 8, 2011

Member

Great. I'll merge it later today unless anyone objects.

Member

takluyver commented Dec 8, 2011

Great. I'll merge it later today unless anyone objects.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 8, 2011

Member

please do, thanks!

Member

minrk commented Dec 8, 2011

please do, thanks!

@takluyver takluyver merged commit 96e8539 into ipython:master Dec 8, 2011

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 8, 2011

Member

Rebased to avoid a merge, and pushed.

Member

takluyver commented Dec 8, 2011

Rebased to avoid a merge, and pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment