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)
Change History (10)
comment:1 Changed 12 years ago by
comment:2 Changed 11 years ago by
#4323 is a duplicate of that ticket.
comment:3 Changed 11 years ago by
- 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.
comment:4 Changed 11 years ago by
(Maybe it only goes from 78.0% to 78.5%.)
comment:5 Changed 11 years ago by
- 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
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.
comment:7 Changed 11 years ago by
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
- 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.
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.