<p style="white-space: pre-wrap; word-wrap: break-word;">Some comments are not related to your change, but rather express my ideas on potential improvements. I could not resist while reading the code.</p><p><a href="https://gerrit.osmocom.org/c/pysim/+/23594">View Change</a></p><p>14 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py">File pySim/utils.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@31">Patch Set #2, Line 31:</a> <code style="font-family:monospace,monospace">str</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"a string of hex nibbles" - Hexstr?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@39">Patch Set #2, Line 39:</a> <code style="font-family:monospace,monospace">str</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"a string of hex nibbles" - Hexstr?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@54">Patch Set #2, Line 54:</a> <code style="font-family:monospace,monospace"># List of bytes to string</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">unrelated: this comment is out of sync with the docstring below:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  "List of bytes" vs "list of integers"</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@113">Patch Set #2, Line 113:</a> <code style="font-family:monospace,monospace">return None</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">A proper Pythonic approach would be to raise exceptions here... So either it returns something, or it fails. Also unrelated, of course.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@157">Patch Set #2, Line 157:</a> <code style="font-family:monospace,monospace">plmn</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hexstr</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@166">Patch Set #2, Line 166:</a> <code style="font-family:monospace,monospace">plmn</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hexstr</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@192">Patch Set #2, Line 192:</a> <code style="font-family:monospace,monospace">fivehexbytes:Hexstr</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I still find it odd that we (ab)use strings to represent bytes in a language where we have a special type for that...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@278">Patch Set #2, Line 278:</a> <code style="font-family:monospace,monospace">->int</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">cosmetic: missing space</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@290">Patch Set #2, Line 290:</a> <code style="font-family:monospace,monospace">  if imsi == None:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Your type hint renders this check useless. I would remove it because it's wrong to pass None and expect this function to decode something. Looks like over-defensive programming to me.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@298">Patch Set #2, Line 298:</a> <code style="font-family:monospace,monospace">long</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">bool</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@302">Patch Set #2, Line 302:</a> <code style="font-family:monospace,monospace"> if imsi == None:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@350">Patch Set #2, Line 350:</a> <code style="font-family:monospace,monospace">ef_msisdn</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hexstr?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@371">Patch Set #2, Line 371:</a> <code style="font-family:monospace,monospace">return None</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we should rather 'raise ValueError' here and drop the 'Optional' from the returned value. Mixing a value, None, and exceptions is one of the worst things that I saw... Just an idea for a separate patch.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@417">Patch Set #2, Line 417:</a> <code style="font-family:monospace,monospace">st</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hexstr? (not sure here, some magic happens on line 432)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/pysim/+/23594">change 23594</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.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.osmocom.org/c/pysim/+/23594"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: pysim </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I50a0a07132890af0817f4ff0ce9fec53b7512522 </div>
<div style="display:none"> Gerrit-Change-Number: 23594 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 03 Apr 2021 23:33:22 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>