fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/26165 )
Change subject: pySim-shell: add method to match card profile to card ...................................................................... Patch Set 5: Code-Review-1 (9 comments) https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim-shell.py File pySim-shell.py: https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim-shell.py@101 PS5, Line 101: if type(profile) is CardProfileUICC or type(profile) is CardProfileUICCSIM: In terms of readability, this is a better approach: if type(profile) in (CardProfileUICC, CardProfileUICCSIM): but given that CardProfileUICCSIM is a child of CardProfileUICC, you better do: if isinstance(profile, CardProfileUICC): which is the right way taking inheritance into account. See https://stackoverflow.com/questions/1549801/what-are-the-differences-between-type-and-isinstance. https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/filesystem.py File pySim/filesystem.py: https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/filesystem.py@1508 PS5, Line 1508: that is overloaded by specific dirived classes 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. https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py File pySim/profile.py: https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py@29 PS5, Line 29: # In order for autodetection ... So AFAIU, the order is important here. And this is why you're not using __subclasses__(). https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py@32 PS5, Line 32: def profile_detect(scc:SimCardCommands): Missing type definition for returned value: -> Optional[CardProfile] https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py@32 PS5, Line 32: profile_detect 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. If we go for this, then we can do something like: profile = CardProfile.pick(scc) 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: def all_subclasses(cls): return set(cls.__subclasses__()).union( [s for c in cls.__subclasses__() for s in all_subclasses(c)]) 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. https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/profile.py@33 PS5, Line 33: ws https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/ts_102_221.py File pySim/ts_102_221.py: https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/ts_102_221.py@631 PS5, Line 631: _match_sim So this is basically a copy of CardProfileSIM.match_with_card()? Also, the only difference from _match_uicc() here seems to be the CTRL byte? You need to generalize this somehow to avoid copy-paste... https://gerrit.osmocom.org/c/pysim/+/26165/5/pySim/ts_102_221.py@746 PS5, Line 746: # Add GSM specific files tabs vs spaces https://gerrit.osmocom.org/c/pysim/+/26165/3/pySim/ts_51_011.py File pySim/ts_51_011.py: https://gerrit.osmocom.org/c/pysim/+/26165/3/pySim/ts_51_011.py@977 PS3, Line 977: def _match_witch_card(scc:SimCardCommands) -> bool: > I see no change or feedback regarding this in the latest patch version 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. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/26165 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: If090d32551145f75c644657b90085a3ef5bfa691 Gerrit-Change-Number: 26165 Gerrit-PatchSet: 5 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillm...@sysmocom.de> Gerrit-Reviewer: fixeria <vyanits...@sysmocom.de> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Comment-Date: Tue, 16 Nov 2021 02:12:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: laforge <lafo...@osmocom.org> Comment-In-Reply-To: dexter <pma...@sysmocom.de> Gerrit-MessageType: comment
