Opened 6 months ago

Closed 6 months ago

#16196 closed defect (fixed)

dedent pasted sage prompts

Reported by: vbraun Owned by:
Priority: major Milestone: sage-6.2
Component: misc Keywords:
Cc: ohanar Merged in:
Authors: R. Andrew Ohana Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 101a6be (Commits) Commit: 101a6beaae0109dffad9a8d93f8ebd44843794e5
Dependencies: Stopgaps:

Description (last modified by kcrisman)

Sage prompts with leading whitespace are not removed correctly:

sage:    1+1     # works
2
sage: sage: 1+1     # works
2
sage:    sage: 1+1     # indent + prompt = fail
  File "<ipython-input-34-360e106e9b2f>", line 1
    sage: Integer(1)+Integer(1)     # indent + prompt = fail
        ^
SyntaxError: invalid syntax

Possibly due to #16050

Change History (12)

comment:1 Changed 6 months ago by kcrisman

  • Description modified (diff)

comment:2 Changed 6 months ago by ohanar

It looks like the change from #16154 broke this (ipython's leading indent stripper is now being run after our prompt stripper). I can look into this sometime later this week.

comment:3 Changed 6 months ago by ohanar

  • Branch set to u/ohanar/ge_prompt_dedents

comment:4 Changed 6 months ago by ohanar

  • Authors set to R. Andrew Ohana
  • Commit set to a1354669efb14821dda9c9c4c5d9361dc7d08bb2
  • Status changed from new to needs_review

This should work for now, although I looked a bit into the upstream code, and that should probably be changed to be more configurable (since after all the prompt is configurable).


New commits:

a135466fix to dedent pasted sage prompts

comment:5 follow-up: Changed 6 months ago by jhpalmieri

Oh, sorry, I didn't see this ticket and introduced #16232 which strips leading whitespace. Do we close that one as a duplicate? (Meanwhile, it has a positive review, by the way.)

  • Why only strip one prompt? I see the explanation that we are trying to match IPython's behavior, but do we need to preserve backward-compatibility?
  • What is the point of the change to src/sage/repl/ipython_extension.py? Just changing the regular expression (as at #16232) seems to solve the problem.

comment:6 in reply to: ↑ 5 Changed 6 months ago by ohanar

Replying to jhpalmieri:

Oh, sorry, I didn't see this ticket and introduced #16232 which strips leading whitespace. Do we close that one as a duplicate? (Meanwhile, it has a positive review, by the way.)

  • Why only strip one prompt? I see the explanation that we are trying to match IPython's behavior, but do we need to preserve backward-compatibility?

Is there sage code that is being written with sage (duplicate) prompts? Really duplicate prompts should only come about by copying something into a terminal session, and then copying that pasted content again -- this seems like a complete edge case that wouldn't (and certainly shouldn't) show up in some code that is meant to be used for a long period of time.

  • What is the point of the change to src/sage/repl/ipython_extension.py? Just changing the regular expression (as at #16232) seems to solve the problem.

IPython already has a dedenter, and it is smart (it strips an equal amount from each line), so all we need to do is strip our prompts after ipython dedents, and for the moment, that means insert after position 0 (and before position 1, so as to not re-break #16154).

Last edited 6 months ago by ohanar (previous) (diff)

comment:7 Changed 6 months ago by ohanar

Like I mentioned, this should just be configurable with IPython itself -- we shouldn't have to be pulling this internal _strip_prompts function in the first place. I'll make a patch if I ever find the time to fix this upstream, but I'm much to busy at the moment.

comment:8 Changed 6 months ago by jhpalmieri

  • Branch changed from u/ohanar/ge_prompt_dedents to u/jhpalmieri/dedent
  • Commit changed from a1354669efb14821dda9c9c4c5d9361dc7d08bb2 to fc520709f1682205ac55f7c75f31eb84d9781587
  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Rebased (w.r.t. #16232).

comment:9 Changed 6 months ago by git

  • Commit changed from fc520709f1682205ac55f7c75f31eb84d9781587 to 101a6beaae0109dffad9a8d93f8ebd44843794e5
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

101a6be#16196: rebased to #16232

comment:10 Changed 6 months ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:11 Changed 6 months ago by leif

Ah, ok... :-)

comment:12 Changed 6 months ago by vbraun

  • Branch changed from u/jhpalmieri/dedent to 101a6beaae0109dffad9a8d93f8ebd44843794e5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.