Opened 12 years ago

Closed 11 years ago

#877 closed defect (fixed)

[with patch, positive review] "sage -coverage" should not care about functions which are local to other functions/methods

Reported by: cwitty Owned by: jhpalmieri
Priority: minor Milestone: sage-4.1.1
Component: documentation Keywords:
Cc: Merged in: Sage 4.1.1.alpha1
Authors: John Palmieri Reviewers: William Stein, Harald Schilly
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Currently, if you have something like:

def foo():
    def bar():
        pass
    pass

then "sage -coverage" will complain that bar() has no docstring or doctests. However, such functions cannot be (directly) doctested, so that warning is invalid. In my opinion, bar() should not be required to have a docstring either.

Attachments (2)

trac_877-scripts-coverage.patch (2.3 KB) - added by jhpalmieri 11 years ago.
apply to scripts repository
trac_877-scripts-coverage2.patch (2.6 KB) - added by jhpalmieri 11 years ago.
use this version instead

Download all attachments as: .zip

Change History (10)

comment:1 Changed 12 years ago by mhansen

To get around this, I took my local function/method and made it a regular one. I then used functools.partial to use it. This allowed my function to be tested like every other one. In the few cases where I had to do this, I ended up liking the functools version better.

comment:2 Changed 11 years ago by zimmerma

#4323 is a duplicate of that ticket.

comment:3 Changed 11 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Owner changed from tba to jhpalmieri
  • Status changed from new to assigned
  • Summary changed from "sage -coverage" should not care about functions which are local to other functions/methods to [with patch, needs review] "sage -coverage" should not care about functions which are local to other functions/methods

Here is a patch for this. With sage-4.1.1.alpha0, it increases overall coverage from 77.9% to 78.5%.

To test: I know that the files steenrod_algebra_element.py and structure/factorization.py have such nested functions, so try 'sage -coverage' on these files, before and after patching.

Changed 11 years ago by jhpalmieri

apply to scripts repository

comment:4 Changed 11 years ago by jhpalmieri

(Maybe it only goes from 78.0% to 78.5%.)

comment:5 Changed 11 years ago by was

  • Summary changed from [with patch, needs review] "sage -coverage" should not care about functions which are local to other functions/methods to [with patch, positive review] "sage -coverage" should not care about functions which are local to other functions/methods

Looks good to me:

BEFORE:

< Overall weighted coverage score:  77.8%
< Total number of functions:  22395

AFTER

> Overall weighted coverage score:  78.3%
> Total number of functions:  22207

The code looks fine and it works fine, so far as I can tell.

comment:6 Changed 11 years ago by schilly

quick note just from looking at the patch: i makes more sense to move the re.compile statement *before* the while True: loop. It's purpose is to speed up the regex searches and shouldn't be compiled in every loop! just exchange line 20 and 21 in the merged file.

Changed 11 years ago by jhpalmieri

use this version instead

comment:7 Changed 11 years ago by jhpalmieri

trac_877-scripts-coverage2.patch interchanges the lines that schilly mentions. It also moves another re.compile statement earlier. It also stores the list of nested functions in the list "closures", for possible future use.

comment:8 Changed 11 years ago by mvngu

  • Merged in set to Sage 4.1.1.alpha1
  • Resolution set to fixed
  • Reviewers set to William Stein, Harald Schilly
  • Status changed from assigned to closed

This is what I observe regarding John's patch. In Sage 4.1 without the patch trac_877-scripts-coverage2.patch, we have

Overall weighted coverage score:  77.8%
Total number of functions:  22398

Applying that patch to Sage 4.1:

Overall weighted coverage score:  78.3%
Total number of functions:  22210

If I understand John's patch correctly, it doesn't count functions which are local to other functions/methods. This accounts for the reduced number of total functions after applying the patch. Moving on to Sage 4.1.1.alpha0 without the patch:

Overall weighted coverage score:  78.0%
Total number of functions:  22497

And with the patch:

Overall weighted coverage score:  78.5%
Total number of functions:  22308

Here is the coverage after applying the patch to my merge tree:

Overall weighted coverage score:  78.6%
Total number of functions:  22317


Merged trac_877-scripts-coverage2.patch in the scripts repository.

Note: See TracTickets for help on using tickets.