-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support running modules #45
Conversation
@joerick can you please review it? |
sure thing. thanks for the contribution! i'll get around to it, just busy at work this week. I'll see if i can take a look at the weekend. thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! comments inline. There are no tests covering this at the moment, I've actually added a couple to master so could you pull that in before continuing? I've added a test with xfail for the module use-case, you can remove that to test your feature.
pyinstrument/__main__.py
Outdated
@@ -4,6 +4,7 @@ | |||
import codecs | |||
from pyinstrument import Profiler | |||
from pyinstrument.profiler import get_recorder_class, get_renderer_class | |||
from importlib.util import find_spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find documentation for this on 2.7.... running on Python 2.7 gives me this:
Traceback (most recent call last):
File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 162, in _run_module_as_main
"__main__", fname, loader, pkg_name)
File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
exec code in run_globals
File "/Users/joerick/04-Open-source-projects/pyinstrument/pyinstrument/__main__.py", line 7, in <module>
from importlib.util import find_spec
ImportError: No module named util
Might need a polyfill for Python 2?
pyinstrument/__main__.py
Outdated
else: | ||
renderer = 'text' | ||
if options.module_name is not None: | ||
progname = find_spec(options.module_name).origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is right - when I run find_spec('pyinstrument').origin
in a shell it gives me the __init__.py
file. It should be running the __main__.py
file as I understand it.
The cProfile module uses runpy for this - it's probably best to copy that.
Changed to runpy and made test succeed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks @iddan !
If you can please post when it is released to PyPI. |
Sorry for the delay - just addressing a bug I found before releasing... I had to make a minor change to this for programs that use ArgumentParser to work. Most programs assume sys.argv has at least one entry. see 2c05990 |
released as 2.1.1! |
Maybe update should be state in README? |
Solves #43. It was a pleasure to get into the codebase. Would suggest adding setting up a venv into the readme under "contributing" or something.