Ticket #3154 (closed defect: fixed)

Opened 5 years ago

Last modified 3 years ago

notebook -- spurious u0027's output

Reported by: was Owned by: boothby
Priority: major Milestone: sage-4.4.2
Component: notebook Keywords:
Cc: Work issues:
Report Upstream: N/A Reviewers: Mitesh Patel, Tim Dumol
Authors: Willem Jan Palenstijn, Tim Dumol Merged in: sagenb-0.8
Dependencies: Stopgaps:

Description

In the notebook we have this, caused by _eval_cmd in worksheet.py: 

%python 
2+2
print "'hi\'"
///

'hi\u0027

Attachments

trac_3154-spurious-u0027-output.patch Download (715 bytes) - added by timdumol 3 years ago.
Uses base64.b64en/decode instead.
3154_escaping_quotes.patch Download (1.4 KB) - added by wjp 3 years ago.
3154_escaping_quotes.2.patch Download (3.2 KB) - added by timdumol 3 years ago.
Fixes a few doctests and a unicode encoding issue.
3154_escaping_quotes.3.patch Download (3.2 KB) - added by mpatel 3 years ago.
Rebase for minor "hunk" failure. Replaces previous.
3154_escaping_quotes.4.patch Download (3.3 KB) - added by mpatel 3 years ago.
Rebased vs. SageNB 0.7.4. Replaces previous.
3154_escaping_quotes.5.patch Download (6.6 KB) - added by mpatel 3 years ago.
Doctest fixes. Replaces all previous.

Change History

Changed 3 years ago by timdumol

Uses base64.b64en/decode instead.

comment:1 Changed 3 years ago by timdumol

  • Status changed from new to needs_review
  • Report Upstream set to N/A
  • Authors set to Tim Dumol

comment:2 Changed 3 years ago by wjp

I think using %r to handle escaping quotes would be cleaner than using intermediate base64. I'm attaching a new patch that does this.

Changed 3 years ago by wjp

comment:3 follow-up: ↓ 5 Changed 3 years ago by mpatel

Can we also use %r instead of base64-encoding in

  • sage.misc.preparser.load_wrap
  • sagenb.misc.support.automatic_name_filter
  • worksheet.Worksheet.preparse

?

comment:4 Changed 3 years ago by mpatel

The second patch causes two SageNB doctest failures: 

File "/home/tmp/sagenb-0.5/src/sagenb/sagenb/notebook/worksheet.py", line 3695:
    sage: W.check_for_system_switching(c1.cleaned_input_text(), c1)
Expected:
    (True, u"print _support_.syseval(gap, ur'''SymmetricGroup(5)''', '...')")
Got:
    (True, u"print _support_.syseval(gap, u'SymmetricGroup(5)', '/home/.sage/temp/chopin/5101/dir_2.sagenb/home/sage/0/cells/1')")
**********************************************************************
File "/home/tmp/sagenb-0.5/src/sagenb/sagenb/notebook/worksheet.py", line 3721:
    sage: W.check_for_system_switching(c1.cleaned_input_text(), c1)
Expected:
    (True, u"print _support_.syseval(gap, ur'''SymmetricGroup(5)''', '...')")
Got:
    (True, "print _support_.syseval(gap, u'SymmetricGroup(5)', '/home/.sage/temp/chopin/5101/dir_2.sagenb/home/sage/0/cells/1')")

Does the latter reveal a [minor] problem with #7249?

comment:5 in reply to: ↑ 3 Changed 3 years ago by timdumol

Replying to mpatel:

Can we also use %r instead of base64-encoding in

  • sage.misc.preparser.load_wrap
  • sagenb.misc.support.automatic_name_filter
  • worksheet.Worksheet.preparse

?

I believe so. I'd rather that be put in a new ticket though.

The attached patch should solve the mentioned doctest problems. Can't see how they're related to #7249 though.

comment:6 Changed 3 years ago by timdumol

Sorry, I forgot to attach the actual patch.

Changed 3 years ago by timdumol

Fixes a few doctests and a unicode encoding issue.

Changed 3 years ago by mpatel

Rebase for minor "hunk" failure. Replaces previous.

comment:7 Changed 3 years ago by mpatel

  • Status changed from needs_review to positive_review
  • Reviewers set to Mitesh Patel
  • Authors changed from Tim Dumol to Willem Jan Palenstijn, Tim Dumol

Nice work! V3 is just a rebase of V2.

comment:8 Changed 3 years ago by timdumol

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sagenb-0.6

Changed 3 years ago by mpatel

Rebased vs. SageNB 0.7.4. Replaces previous.

comment:9 Changed 3 years ago by mpatel

  • Status changed from closed to needs_work
  • Work issues set to Rebase and fix tests
  • Merged in sagenb-0.6 deleted

comment:10 Changed 3 years ago by mpatel

V4 is rebased against SageNB 0.7.4 (cf. #8051), but now several doctests fail, at least one of which I can't investigate lucidly right now. I'll return to this soon.

comment:11 Changed 3 years ago by mpatel

I've reopened this ticket because its not in SageNB 0.7 (cf. #8051). I mistakenly thought, per hg log, that I had merged it, but I really merged #4217, whose commit string refers to #3154.

Changed 3 years ago by mpatel

Doctest fixes. Replaces all previous.

comment:12 Changed 3 years ago by mpatel

  • Status changed from needs_work to needs_review
  • Work issues Rebase and fix tests deleted

V5 is rebased for SageNB 0.7.4 and it includes several new doctest fixes. Can someone review my changes?

comment:13 Changed 3 years ago by timdumol

  • Status changed from needs_review to positive_review
  • Reviewers changed from Mitesh Patel to Mitesh Patel, Tim Dumol

Doctests pass, no regressions noted.

comment:14 Changed 3 years ago by davidloeffler

  • Milestone changed from sage-4.3.1 to sage-4.4

comment:15 Changed 3 years ago by timdumol

  • Merged in set to sagenb-0.8

comment:16 Changed 3 years ago by timdumol

  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.