There isn't really enough data to decide. * Does testing A and B provide full coverage of the helper? * Is the helper performance-critical? * Do you need Cython? * If no, why is the code in that module at all? Source files in Sage are almost always too long to comfortably read, the default should be to create a new one for new functionality. * If yes, why not cpdef so people later can import it into the command line
In general, doctests are useful for interactive use but often don't make much sense for more tightly coupled code (unit test work better there). We do have the policy of having doctests everywhere as an attempt at enforcing code coverage. Its certainly not perfect. Requiring 100% line coverage would certainly be better, but we don't have the test infrastructure for that. On Friday, August 14, 2015 at 8:47:00 PM UTC+2, Dima Pasechnik wrote: > > I have the following Cython design under review: > ------------------- > cdef helper(x): > .... > > def A(..): > return helper(foo) > > def B(..): > return helper(blah) > > ------------------- > > A and B are properly documented, doctested, etc etc > The reviewer insists that helper() must be turned into pure Python > function and > doctested on its own, while I find this not needed, as tests in A and B > suffice. > > He accuses me of using cdef just because I do not want to write doctests. > The module in question already has a number of cdef'd functions without > doctests, written > by the reviewer and positively reviewed by me some time ago. > When I point this out to the reviewer, I hear that these functions must be > fast, so this is why > they are cdef'd. > > Your opinions? > > Thanks, > Dima > > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
