Conversation
|
Just took a cursory glance, seems that you also unnested a lot of functions. Will take a closer look later today. One maybe unintended side effect though is that all these functions are now in the global namespace, and will show in the docs because we have in the Sphinx boilerplate .. automodule:: <module>
:private-members:I wonder if we should just remove that in |
|
Yeah, that probably makes sense to remove private members from docs. |
| ('--view', # Verbosity level 1 = `--view` | ||
| {'^Output to stdout': True, | ||
| r"^Wrote .* '.*script\.py\.lprof'": True, | ||
| r'Parser output:''(?:\n)+'r'.*namespace\((?:.+,\n)*.*\)': False, |
There was a problem hiding this comment.
Guess that we can do the same for the 1st parametrization.. but of course I can also just put that in after merging.
There was a problem hiding this comment.
I thought that I had removed the log associated with this test. If it's a mistake you can put it back in.
| STATIC_ANALYSIS = _boolean_environ('LINE_PROFILER_STATIC_ANALYSIS') | ||
|
|
||
| log = _logger.Logger('line_profiler') | ||
| log = _logger.Logger('line_profiler', backend='auto') |
There was a problem hiding this comment.
I thought this is the default? Doesn't hurt to be explicit though, of course.
There was a problem hiding this comment.
yeah, I'm just being explicit. The logger gets initialized twice when kernprof is used, which is why I changed it.
| if options.rich and simple_logging: | ||
| from rich.console import Console | ||
| from rich.markup import escape | ||
| def no_op(*_, **__) -> None: |
There was a problem hiding this comment.
Just caught that this is the only annotated function in the entire file (my fault, it has been here since before your PR). Maybe we should remove that for consistency...
|
|
||
|
|
||
| def _write_preimports(prof, options, exclude): | ||
| def _gather_preimport_targets(options, exclude): |
There was a problem hiding this comment.
Good call breaking up _write_preimports, it was indeed getting a bit long...
| 'Wrote temporary module for pre-imports ' | ||
| f'to {temp_mod_path!r}:', | ||
| code) | ||
| f'to {temp_mod_path!r}:') |
There was a problem hiding this comment.
We should probably remove the colon now that we're no longer outputting the module to the log
| after initial parsing of options; not to be invoked on its own. | ||
| """ | ||
| script_file, prof = _pre_profile(options, module) | ||
| try: |
There was a problem hiding this comment.
I know this is old code but we might as well discuss this now, do you think we can either:
- Unnest the inner
try: ... except: ...block so we just have one singletry: ... except: ... finally: ...block, or - Remove the
except: ...clause so that we don't suppress thoseBaseExceptions?
There was a problem hiding this comment.
I haven't really thought through what the implications of the double try nesting is. Looking at git blame, that is really old code from 2008, so I imagine we can spruce it up.
TTsangSC
left a comment
TTsangSC
left a comment
There was a problem hiding this comment.
Looks great, thanks for the PR. If you think it's ready I'll pull, (make the aforementioned small changes), and push to pyutils#337.
|
I think all you should need to do is hit merge, and pyutils#337 will update. |
2 participants
Based on my review, I have a refactoring / modification of several components.
The idea is to:
functools.partialwhen possibleoptionswhen possible (although I had to eventually do some of this so the refactor wouldn't blow up)