<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8689">View Change</a></p><p>Patch set 6:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p style="white-space: pre-wrap; word-wrap: break-word;">The -1 is for the sipp.py comment only.  All other findings can be deferred if you want.</p><p>(7 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/self_test/test_utils_socket.py">File lib/python/asterisk/self_test/test_utils_socket.py:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/self_test/test_utils_socket.py">Patch Set #6:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">pep8 produces many warnings:<br>E731 do not assign a lambda expression, use a def</p><p style="white-space: pre-wrap; word-wrap: break-word;">If you want to fix it great if not that's fine we can get it later.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/self_test/test_utils_socket.py@14">Patch Set #6, Line 14:</a> <code style="font-family:monospace,monospace">import os</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">os is unused in this source.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/self_test/test_utils_socket.py@21">Patch Set #6, Line 21:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">sys.path.append(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))<br>from utils_socket import Ports, PortError, get_available_port, MIN_PORT<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The rest of the tests I'm working on use code as follows:<br>sys.path.append('lib/python')  # noqa</p><p style="white-space: pre-wrap; word-wrap: break-word;">from asterisk.utils_socket import Ports, PortError, get_available_port, MIN_PORT</p><p style="white-space: pre-wrap; word-wrap: break-word;"><br>Technically nothing wrong with what you've done here although it is confusing because the 2 os.path.dirname actually adds lib/python/asterisk to sys.path.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The '  # noqa' (two spaces before hash) cause that line to be ignored by pep8.  This suppresses a warning about imports being</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/sipp.py">File lib/python/asterisk/sipp.py:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/sipp.py@685">Patch Set #6, Line 685:</a> <code style="font-family:monospace,monospace">                config=default_args, num=5))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Do we actually need 5 ports?  I thought we only needed 4:<br>audio rtp<br>audio rtcp<br>video rtp<br>video rtcp</p><p style="white-space: pre-wrap; word-wrap: break-word;">What is the 5th port used for?</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py">File lib/python/asterisk/utils_socket.py:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py@1">Patch Set #6, Line 1:</a> <code style="font-family:monospace,monospace">#!/usr/bin/env python</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please remove this for clarity that it is not an executable script.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py@14">Patch Set #6, Line 14:</a> <code style="font-family:monospace,monospace">import sys</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Unused import</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8689/6/lib/python/asterisk/utils_socket.py@192">Patch Set #6, Line 192:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">        if not isinstance(ports, list):<br>            ports = [ports]<br></pre></blockquote></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">This can be made compatible with python3 by:<br>if isinstance(ports, int):<br>    ports = [ports]</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if not isinstance(ports, list):<br>    ports = list(ports)</pre><p style="white-space: pre-wrap; word-wrap: break-word;">In python3 it is an error if isinstance(ports, range), but this cannot be tested directly in python2.  I'm unsure this may be an issue for Ports.reserve() as well, that is not hit by the unit tests.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8689">change 8689</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/8689"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: testsuite </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I3da461123afc30e1f5ca12e65d289eaa42d6de00 </div>
<div style="display:none"> Gerrit-Change-Number: 8689 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 19 Apr 2018 20:32:03 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>