Lint and type hints for REPL #1364
Conversation
There was a problem hiding this comment.
@dhoomakethu can you please review the REPL parts carefully, I am not sure if there are side effects of these changes.
The SImulator changes are OK.
|
|
||
| import click |
There was a problem hiding this comment.
This will not work !!! the import needs to be behind a try/except, otherwise you require click to be installed always (you will see the same in client/serial and other places).
There was a problem hiding this comment.
The import is only needed for REPL, which can't be used anyway without click. the existing code which I removed calls sys.exit(1)
pymodbus/pymodbus/repl/client/main.py
Lines 9 to 13 in d2c7c77
There was a problem hiding this comment.
Yes but we have had problems, latest with sqlalchemy because python loads the files and thus tries to do the import. The problem arises is the package is referenced (jnit) so for optional libraries we prefer to use try/except.
Having the try/except allows python to pass the module without having click installed (which is optional).
There was a problem hiding this comment.
This is the "modern" way we handle optional packages:
try:
import serial
except ImportError:
pass
There was a problem hiding this comment.
#1114 was the import error with pyserial, which was caused because it was imported in the generic server class. This is called by the normal server/client code paths.
#1319 was the import error with sqlalchemy and redis, which was caused because they were imported in the datastore. This is called by the normal server/client code paths.
I don't think this is true here, since no other code in the normal server/client code paths calls the REPL. If something did, it would already trigger the (existing) sys.exit(1). I have tested using the pymodbus client without click installed, and it's fine. So the Try/Except is not protecting anything.
As far as I can tell, if click is not installed the REPL is useless. So the Try/Except only changes the ImportError to a NameError.
I can add the Try/Except, of course, but don't think it has any use.
There was a problem hiding this comment.
Your analysis is correct, I was merely preparing for the future where init contain all external classes, and deep links are not allowed.
Lets leave it for now.
|
|
||
| import click | ||
| from prompt_toolkit import PromptSession, print_formatted_text |
There was a problem hiding this comment.
This should also be in try/except.
|
Once the try/except is in place (in the modern way) I will approve/merge. Of course if dhoomakethu have comments before that they should be addressed, if not they can be addressed later. |
|
As you might have noticed I work heavily in pymodbus/client (mainly base.py), so please do not make a lot of changes in that directory. |
These are the current mypy problems there. I will let your work settle down before touching it. |
* avoid re-using `file` variable * Click version >=8 dropped this warning for Python2 * Make `click` an explicit requirement * annotations is already part of Python3.8 * fix flake8 warnings * avoid redefining extra_args * Fix mangled docstring * Use slave argument * Give default values, and fix values/write_registers mismatch * `list` is native type, `List[...]` is a type hint --------- Co-authored-by: jan iversen <jancasacondor@gmail.com>
This removes all the
mypywarnings for REPL, and eliminates a couple other lints also.