Opened 13 years ago

Last modified 10 years ago

#8708 closed enhancement

allow doctest script to handle docstrings with triple single quotes — at Version 9

Reported by: Minh Van Nguyen Owned by: tbd
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: doctest coverage Keywords: sd41
Cc: Niels Ranosch Merged in:
Authors: John Palmieri Reviewers: Burcin Erocal
Report Upstream: N/A Work issues: Rebase on #9739 (and perhaps also 10952).
Branch: Commit:
Dependencies: #9739 Stopgaps:

Status badges

Description (last modified by John Palmieri)

As the subject says. This is related to #8699. See this sage-devel thread for some background information.


Apply only trac_8708-jhp.v2.patch to the scripts repository.

Change History (12)

Changed 11 years ago by Burcin Erocal

comment:1 Changed 11 years ago by Burcin Erocal

Authors: Burcin Erocal
Cc: Niels Ranosch added
Milestone: sage-4.7.1sage-4.7.2
Status: newneeds_review

comment:2 Changed 11 years ago by John Palmieri

Authors: Burcin ErocalBurcin Erocal, John Palmieri

This doesn't work for me: it skips all doctests in a typical file. The issue is the line

min_ind = min(dq_ind, sq_ind)

which means that if either (not just both) search fails, then min_ind is -1, so the loop exits.

I also don't think we need to modify doc_prefix and doc_suffix, since this is part of the string written to the temporary doctesting file. Do the triple quotes in that file have to match the original ones, or can they all be """?

I'm attaching a new patch which uses regular expressions instead, and which doesn't modify doc_prefix and doc_suffix. I'm also attaching a small file for testing purposes; testing on this file should produce three failures, and if you add some print statements to the function extract_doc, it should extract the correct string.

comment:3 in reply to:  2 Changed 11 years ago by Burcin Erocal

Replying to jhpalmieri:

This doesn't work for me: it skips all doctests in a typical file. The issue is the line

min_ind = min(dq_ind, sq_ind)

which means that if either (not just both) search fails, then min_ind is -1, so the loop exits.

Good catch! Thanks for looking into this.

I also don't think we need to modify doc_prefix and doc_suffix, since this is part of the string written to the temporary doctesting file. Do the triple quotes in that file have to match the original ones, or can they all be """?

What happens if you have something like:

def f(arg):
    '''
    some text
        
    """
    some more text
    """

    '''

comment:4 Changed 11 years ago by John Palmieri

Oh, you're right. I'm replacing my patch with a new one, the difference being

  • sage-doctest

    diff --git a/sage-doctest b/sage-doctest
    a b def change_warning_output(file): 
    452452            tmpfiles.append(os.path.join(SAGE_TESTDIR, name + '.pyc'))
    453453
    454454    # Prefix/suffix for all doctests replacing the starting/ending """
    455     doc_prefix = 'r""">>> set_random_seed(0L)\n\n>>> change_warning_output(sys.stdout)\n\n'
    456     doc_suffix = '\n>>> sig_on_count()\n0\n"""'
     455    doc_prefix_dq = 'r""">>> set_random_seed(0L)\n\n>>> change_warning_output(sys.stdout)\n\n'
     456    doc_prefix_sq = doc_prefix_dq.replace('"""',"'''")
     457    doc_suffix_dq = '\n>>> sig_on_count()\n0\n"""'
     458    doc_suffix_sq = doc_suffix_dq.replace('"""',"'''")
    457459
    458460    n = 0
    459461    # Now extract the docstring by using a regular expression search
    def change_warning_output(file): 
    499501            name = "example"
    500502            s += "def %s_%s():"%(name,n_str)
    501503            n += 1
    502             s += "\t"+ doc_prefix + doc + doc_suffix + "\n\n"
     504            if m.groups()[0].find("'") > -1: # single quotes
     505                s += "\t"+ doc_prefix_sq + doc + doc_suffix_sq + "\n\n"
     506            else:
     507                s += "\t"+ doc_prefix_dq + doc + doc_suffix_dq + "\n\n"
    503508
    504509    if n == 0:
    505510        return  ''

I'll also add an example like this to the testing file.

Changed 11 years ago by John Palmieri

Attachment: trac_8708-jhp.patch added

apply to scripts repo

Changed 11 years ago by John Palmieri

Attachment: trac_8708-testing.py added

small Python file for testing purposes

comment:5 Changed 11 years ago by Burcin Erocal

Authors: Burcin Erocal, John PalmieriJohn Palmieri
Reviewers: Burcin Erocal
Status: needs_reviewpositive_review

Looks good to me. Thanks for looking into this John.

comment:6 Changed 11 years ago by Leif Leonhardy

Description: modified (diff)

comment:7 Changed 11 years ago by Leif Leonhardy

Dependencies: #9739
Status: positive_reviewneeds_work
Work issues: Rebase on #9739 (and perhaps also 10952).

This needs to be rebased on #9739, and perhaps also #10952 (both merged into Sage 4.7.2.alpha3).

comment:8 Changed 11 years ago by John Palmieri

Priority: minorblocker
Status: needs_workneeds_review

We missed an or two issue in #9739: with non-library files, first we just had a bug, using "source" instead of "file_name" or "root_name" or something. Second, in the import statement added to the file,

s += "from %s import *\n\n" % target_name

we need to replace target_name with os.path.basename(target_name).

In the new patch, I've made these two changes and also rebased. Given that there is actually new content, this needs a new review. Without the new content, doctesting .py non-library files completely fails, so I'm marking this as a blocker. Feel free to downgrade if you think it's appropriate.

Oh, I also realized that the attached file for testing purposes has a bad name: replace the hyphen with an underscore.

The part needing review:

  • sage-doctest

    diff --git a/sage-doctest b/sage-doctest
    a b def check_with_tolerance(expected, actua 
    510510
    511511        elif ext in ['.py', '.sage']:
    512512
    513             target_name = "%s_%d" % (file_name, os.getpid()) # like 'name', but
     513            target_name = "%s_%d" % (root_name, os.getpid()) # like 'name', but
    514514            target_base = os.path.join(SAGE_TESTDIR, target_name) # like 'base'
    515515
    516516            if ext == '.sage':
    def check_with_tolerance(expected, actua 
    528528                # TODO: instead of copying the file, add its source
    529529                # directory to PYTHONPATH.  We would also have to
    530530                # import from 'name' instead of 'target_name'.
    531                 os.system("cp '%s' %s.py" % (source, target_base))
     531                os.system("cp '%s' %s.py" % (file_name, target_base))
    532532
    533             s += "from %s import *\n\n" % target_name
     533            s += "from %s import *\n\n" % os.path.basename(target_name)
    534534
    535535            tmpfiles.append(target_base + ".py") # preparsed or copied original
    536536            tmpfiles.append(target_base + ".pyc") # compiled version of it

comment:9 Changed 11 years ago by John Palmieri

Description: modified (diff)
Note: See TracTickets for help on using tickets.