<p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/pysim/+/26165">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim-shell.py">File pySim-shell.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/+/26165/5/pySim-shell.py@101">Patch Set #5, Line 101:</a> <code style="font-family:monospace,monospace">        if type(profile) is CardProfileUICC or type(profile) is CardProfileUICCSIM:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">In terms of readability, this is a better approach:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if type(profile) in (CardProfileUICC, CardProfileUICCSIM):</pre><p style="white-space: pre-wrap; word-wrap: break-word;">but given that CardProfileUICCSIM is a child of CardProfileUICC, you better do:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if isinstance(profile, CardProfileUICC):</pre><p style="white-space: pre-wrap; word-wrap: break-word;">which is the right way taking inheritance into account. See https://stackoverflow.com/questions/1549801/what-are-the-differences-between-type-and-isinstance.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/filesystem.py">File pySim/filesystem.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/+/26165/5/pySim/filesystem.py@1508">Patch Set #5, Line 1508:</a> <code style="font-family:monospace,monospace">that is overloaded by specific dirived classes</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If I understand correctly, this is an *abstract* static method. In Python you should mark it as such using the '@abc.abstractmethod', so it would be impossible to inherit this class without defining the child-specific implementation of this method. See examples below, e.g. CardModel.add_files(), which is an abstract class method.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py">File pySim/profile.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/+/26165/5/pySim/profile.py@29">Patch Set #5, Line 29:</a> <code style="font-family:monospace,monospace"># In order for autodetection ...</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So AFAIU, the order is important here. And this is why you're not using __subclasses__().</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py@32">Patch Set #5, Line 32:</a> <code style="font-family:monospace,monospace">def profile_detect(scc:SimCardCommands):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing type definition for returned value:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  -> Optional[CardProfile]</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py@32">Patch Set #5, Line 32:</a> <code style="font-family:monospace,monospace">profile_detect</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I find it confusing that you create a new module called 'profile', which itself does not define class CardProfile, but provides some API for this class. Ideally, I would expect to see this API as a @classmethod of the CardProfile.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If we go for this, then we can do something like:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  profile = CardProfile.pick(scc)</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and it looks logical: CardProfile (being an abstract class) picks a suitable child for the given card. Indeed, we would have to use the __subclasses__(). The problem with sub-classes of sub-classes you mentioned can be solved by doing recursion:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  def all_subclasses(cls):<br>      return set(cls.__subclasses__()).union(<br>          [s for c in cls.__subclasses__() for s in all_subclasses(c)])</pre><p style="white-space: pre-wrap; word-wrap: break-word;">The problem with specific ordering is more critical, because this is an implementation specific detail of Python; thus it may (actually does) vary from one version to another. You can solve it by assigning some kind of priority to all children of the CardProfile, so that would then allow you to sort() them.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py@33">Patch Set #5, Line 33:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/ts_102_221.py">File pySim/ts_102_221.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/+/26165/5/pySim/ts_102_221.py@631">Patch Set #5, Line 631:</a> <code style="font-family:monospace,monospace">_match_sim</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So this is basically a copy of CardProfileSIM.match_with_card()?<br>Also, the only difference from _match_uicc() here seems to be the CTRL byte?<br>You need to generalize this somehow to avoid copy-paste...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/ts_102_221.py@746">Patch Set #5, Line 746:</a> <code style="font-family:monospace,monospace">     # Add GSM specific files</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">tabs vs spaces</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/pysim/+/26165/3/pySim/ts_51_011.py">File pySim/ts_51_011.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/+/26165/3/pySim/ts_51_011.py@977">Patch Set #3, Line 977:</a> <code style="font-family:monospace,monospace">def _match_witch_card(scc:SimCardCommands) -> bool:</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I see no change or feedback regarding this in the latest patch version</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I agree with Harald here. This principle applies to functions: it's easier to read shorter ones doing one specific thing. But for classes... I don't think you win anything; rather loose in terms of the code structure.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/pysim/+/26165">change 26165</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/+/26165"/><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: If090d32551145f75c644657b90085a3ef5bfa691 </div>
<div style="display:none"> Gerrit-Change-Number: 26165 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 16 Nov 2021 02:12:46 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>