PromptManager fixes #1078

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@minrk
Member

minrk commented Dec 1, 2011

  • InteractiveShell.prompt_foo traits show deprecation warning, and map to new PromptManager traits
  • PromptManager properly added to IPython App, so it will show up in config
  • add helpstrings to PromptManager traits.
  • Docs / embed references to Shell.prompt_foo also updated

Should close #1075

PromptManager fixes
* InteractiveShell.prompt_foo traits show deprecation warning, and map to new PromptManager traits
* PromptManager properly added to IPython App, so it will show up in config
* add helpstrings to PromptManager traits.
* Docs / embed references to Shell.prompt_foo also updated
@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 1, 2011

Member

Code looks good, but before we close #1075, as Gokhan says, we're inserting weird space before the prompt with these other templates, I'm not sure why. I tried putting simply

c.PromptManager.in_template = r'>>> '
c.PromptManager.out_template = r''

but I get this output:

    >>> 3
        3

I haven't looked at the prompt display logic in ages, but there's something odd here...

Member

fperez commented Dec 1, 2011

Code looks good, but before we close #1075, as Gokhan says, we're inserting weird space before the prompt with these other templates, I'm not sure why. I tried putting simply

c.PromptManager.in_template = r'>>> '
c.PromptManager.out_template = r''

but I get this output:

    >>> 3
        3

I haven't looked at the prompt display logic in ages, but there's something odd here...

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 1, 2011

Member

Ah, never mind.. I see the issue with the justification. Thanks. But that makes me think something...

Member

fperez commented Dec 1, 2011

Ah, never mind.. I see the issue with the justification. Thanks. But that makes me think something...

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 1, 2011

Member

That this means the rewrite logic is a little flawed in @takluyver's rework, and I missed that. The rewrite prompt in the old code would be the one adjusting dynamically its length to that of the others, not the other way around. So users should be able to define prompts of any length they want, and the rewrite prompt will simply be '-------.... ---->' , as long as it needs to be to match the length of the prompt above it. I went to great lengths to make this work in the presence of ansi escapes.

It seems that now the rewrite prompt has a fixed length and the others ares simply justified to match it. That's definitely not the logic we want; it's ugly and it breaks the common case of people changing their prompts and getting that weird left-space.

I'm actually OK with the rewrite prompt not being a template at all, we're simply saying 'the line above got rewritten'. The rewrite prompt instead could be a simple boolean: if True, we rewrite with '---->' as long as it needs to be, if False we don't show any rewrite. There's no need for having a template there.

Sorry that I didn't realize this slipped in when reviewing #507; I was trying to flush so much code to get to the beta that I failed to see these issues. But we have time to fix them before release...

Member

fperez commented Dec 1, 2011

That this means the rewrite logic is a little flawed in @takluyver's rework, and I missed that. The rewrite prompt in the old code would be the one adjusting dynamically its length to that of the others, not the other way around. So users should be able to define prompts of any length they want, and the rewrite prompt will simply be '-------.... ---->' , as long as it needs to be to match the length of the prompt above it. I went to great lengths to make this work in the presence of ansi escapes.

It seems that now the rewrite prompt has a fixed length and the others ares simply justified to match it. That's definitely not the logic we want; it's ugly and it breaks the common case of people changing their prompts and getting that weird left-space.

I'm actually OK with the rewrite prompt not being a template at all, we're simply saying 'the line above got rewritten'. The rewrite prompt instead could be a simple boolean: if True, we rewrite with '---->' as long as it needs to be, if False we don't show any rewrite. There's no need for having a template there.

Sorry that I didn't realize this slipped in when reviewing #507; I was trying to flush so much code to get to the beta that I failed to see these issues. But we have time to fix them before release...

@@ -155,8 +155,8 @@ Prompt customization
The sh profile uses the following prompt configurations::
- o.prompt_in1= r'\C_LightBlue[\C_LightCyan\Y2\C_LightBlue]\C_Green|\#>'
- o.prompt_in2= r'\C_Green|\C_LightGreen\D\C_Green>'
+ o.PromptManager.in_template= r'\C_LightBlue[\C_LightCyan\Y2\C_LightBlue]\C_Green|\#>'

This comment has been minimized.

@takluyver

takluyver Dec 1, 2011

Member

These colour specifications won't work. See my changes to the pysh config in 00cffc4.

@takluyver

takluyver Dec 1, 2011

Member

These colour specifications won't work. See my changes to the pysh config in 00cffc4.

@takluyver takluyver referenced this pull request Dec 1, 2011

Merged

Prompts #1083

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 1, 2011

Member

Made PR #1083.

Member

takluyver commented Dec 1, 2011

Made PR #1083.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 1, 2011

Member

superseded by #1083

Member

minrk commented Dec 1, 2011

superseded by #1083

@minrk minrk closed this Dec 1, 2011

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