Alexander Belopolsky <[email protected]> added the comment:
Comments on issue9315.27-maint.5.patch:
1. I think you forgot to svn add Lib/test/__init__.py.
2. Instead of "import tracedmodules.testmod", please use something like "from
test.tracedmodules import testmod". There is no need to use implicit relative
import and this is different from accepted practice.
3.
def traced_func_importing(aa, bb):
from tracedmodules.testmod import func
Same comment as in #2 above. No need for implicit relative import here. Also
I am not sure it is interesting to test tracing the import statement inside a
function. Tests for tracing calls to imported functions are good, but I am not
sure import itself generates any interesting events. Just give it a thought -
I don't have an informed opinion.
4. Shouldn't all traced functions go to tracedmodules?
5. Please add comments to functions used for line tracing that changing
relative or absolute (if matters) line numbers will break the tests.
6. Add comments to testmod.py.
7. You lost changes I made in issue9315.4-release27.patch. Specifically, using
trailing underscores or double letters to resolve conflicts between variable
names is not common style. Trailing underscore convention is for resolving
conflicts with python keywords.
8. Please rewrap text in README to fit in 80 columns. In fact, this text
belongs to __init__.py docstring and the comment about importance of function
location should go next to each (currently one) function for which location is
important.
9. fix_pyc_ext(filename) description is misleading. It does not care about
incoming filename extension and just whatever extension with '.py'. This is
probably good because it works with both '.pyc' and '.pyo', but the name and
the docstring suggest otherwise. Note the similar logic in the trace module
itself is implemented as follows:
if filename.endswith((".pyc", ".pyo")):
filename = filename[:-1]
I also feel that three functions to just compute ('test_trace.py',
'test_trace') tuple is an overkill. Please look in the inspect module for
possible alternatives. Also rather than recomputing these strings in each test
case, I would just assign them to module global variables say THIS_FILE_NAME
and THIS_MODULE_NAME.
10. A nitpick. I don't think I've ever seen test_main() function called
"Driver" in the python test suite. Please try to keep consistency in
terminology and coding style between the test modules to the extent it is
practical.
11. Similar to #10. I've changed 'ZZZ' to 'XXX' in
issue9315.4-release27.patch, but you lost that change. See msg112230 above.
----------
_______________________________________
Python tracker <[email protected]>
<http://bugs.python.org/issue9315>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
