Patch Set 2: Code-Review-1
(6 comments)
Looking at this patch I'm almost certain that you haven't tested it
sufficiently with both py2 and py3.
What is your root motivation to make this usable by py3? Doing so is not
trivial, especially concerning encoding issues, and also adding braces to print
only works as long as there is a single argument to print...
We definitely must not break py2 support here, adding py3 seems to be just a
gimmick we can live without?
My personal choice here is to move to the osmo_interact_{vty,ctrl}.py /
osmo_verify_transcript_*.py code which is firmly py3, and using the transcript
file way saves a lot of py coding while writing tests. More complex tests are
also possible by using the API directly.
https://gerrit.osmocom.org/#/c/5038/2/osmopy/obscvty.py
File osmopy/obscvty.py:
Line 96: print("Socket: in %d tries, connected to %s:%d %r (%d
sockets open)" % ())
-1: this can't be right
https://gerrit.osmocom.org/#/c/5038/2/osmopy/osmo_ctrl.py
File osmopy/osmo_ctrl.py:
Line 25: # FIXME: use argparse instead of deprecated optparse
I doubt the value of this comment
Line 67: print("Got message:", Ctrl().rem_header(head))
-1: this won't work well in py2, it will print a tuple. same below
https://gerrit.osmocom.org/#/c/5038/2/osmopy/osmodumpdoc.py
File osmopy/osmodumpdoc.py:
Line 51: print("Skipping app %s" % appname, file=sys.stderr)
no py2, multiple times
https://gerrit.osmocom.org/#/c/5038/2/osmopy/osmotestconfig.py
File osmopy/osmotestconfig.py:
Line 67: print("Config was\n%s" % open(config).read(), file=sys.stderr)
no
Line 129: cmd_line, '\n'.join(err_lines)), file=sys.stderr)
no
--
To view, visit https://gerrit.osmocom.org/5038
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I80e5850a8978d78cda793e2192ef4bd3fd54a121
Gerrit-PatchSet: 2
Gerrit-Project: python/osmo-python-tests
Gerrit-Branch: master
Gerrit-Owner: Max <[email protected]>
Gerrit-Reviewer: Harald Welte <[email protected]>
Gerrit-Reviewer: Neels Hofmeyr <[email protected]>
Gerrit-Reviewer: Pau Espin Pedrol <[email protected]>
Gerrit-HasComments: Yes
