Defer certain imports #413
Conversation
- atexit - codecs - signal - tempfile - copy
- datetime - functools - io - subprocess - traceback
# Conflicts: # cmd2/cmd2.py # tests/test_completion.py # tests/test_submenu.py
Codecov Report
@@ Coverage Diff @@
## master #413 +/- ##
=========================================
+ Coverage 89.2% 89.2% +<.01%
=========================================
Files 8 9 +1
Lines 2594 2613 +19
=========================================
+ Hits 2314 2331 +17
- Misses 280 282 +2
Continue to review full report at Codecov.
|
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Consider my few substantive comments. But everything looks good to me.
| setattr(func, HELP_CATEGORY, category) | ||
|
|
||
|
|
||
| def _which(editor: str) -> Optional[str]: |
There was a problem hiding this comment.
Thanks for moving this to utils.py, that is a more appropriate location for it
| method. Default passes a string of whatever the user typed. | ||
| With this decorator, the decorated method will receive a list | ||
| of arguments parsed from user input using shlex.split().""" | ||
| import functools |
There was a problem hiding this comment.
I don't know enough about how Python importing works ...
If we call this function 1000 times, it just gets imported once right? i.e. there isn't a performance penalty here is there?
Also, if we then call the function below and hit the same import on line 184 below after calling this function, it doesn't import it again there right?
There was a problem hiding this comment.
Correct. https://docs.python.org/3/reference/import.html#the-module-cache describes how the import machinery first looks to see if the module is already imported, if so, it just uses the module already in the cache. Think of "import functools" as "import functools if it hasn't already been imported"
cmd2/cmd2.py
Outdated
|
|
||
| Git repository on GitHub at https://github.com/python-cmd2/cmd2 | ||
| """ | ||
| # many imports are lazy-loaded when they are needed |
There was a problem hiding this comment.
Maybe briefly comment on why this is being done?
cmd2/cmd2.py
Outdated
| import sys | ||
| import tempfile | ||
| import traceback | ||
| from typing import Callable, List, Optional, Union, Tuple |
There was a problem hiding this comment.
I think Optional is now an unused import
| setattr(self.obj, attrib, getattr(self, attrib)) | ||
|
|
||
|
|
||
| class OutputTrap(object): |
There was a problem hiding this comment.
Thanks for moving all of the transcript testing stuff to its own file. cmd2.py is now feeling a little more focused
| @@ -0,0 +1,213 @@ | |||
| # | |||
There was a problem hiding this comment.
Maybe add a brief comment regarding the overall purpose of the code in this file?
|
|
||
| from cmd2 import cmd2 | ||
|
|
||
| import cmd2 |
There was a problem hiding this comment.
This general approach of importing looks nice and clean to me
| def do_aprint(self, statement): | ||
| """Print the argument string this basic command is called with.""" | ||
| self.poutput('aprint was called with argument: {!r}'.format(arg)) | ||
| self.poutput('aprint was called with argument: {!r}'.format(statement)) |
| @@ -0,0 +1,99 @@ | |||
| # Speedup Import | |||
There was a problem hiding this comment.
In the long run I don't think we want this file at the top level. But it should be fine for now (until we are happy we have the issue resolved).
| return current | ||
|
|
||
| def which(editor: str) -> Optional[str]: | ||
| import subprocess |
There was a problem hiding this comment.
Maybe add a docstring comment? I know the original probably didn't have one, but now that it is separated from its immediate use it might be helpful.
In an attempt to resolve #369, this PR:
cmd2/__init__.pyso those modules and functions are available when you import cmd2. In between version 0.8.5 and where master is now, we removed those imports to speed up the wall time for import cmd2, which also broke backwards compatibilitycmd2.pyso there is less code to parse every time you import cmd2CHANGELOG.mdabout the still unresolved The default imports in __init__ causes significant lag #369.#369 is still not resolved, and we continue to work that issue, but will go ahead with 0.9.0 without resolving.