Change in pysim[master]: utils.py: Add more type annotations

fixeria gerrit-no-reply at lists.osmocom.org
Sat Apr 3 23:33:22 UTC 2021


fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/23594 )

Change subject: utils.py: Add more type annotations
......................................................................


Patch Set 2:

(14 comments)

Some comments are not related to your change, but rather express my ideas on potential improvements. I could not resist while reading the code.

https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py 
File pySim/utils.py:

https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@31 
PS2, Line 31: str
"a string of hex nibbles" - Hexstr?


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@39 
PS2, Line 39: str
"a string of hex nibbles" - Hexstr?


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@54 
PS2, Line 54: # List of bytes to string
unrelated: this comment is out of sync with the docstring below:

  "List of bytes" vs "list of integers"


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@113 
PS2, Line 113: return None
A proper Pythonic approach would be to raise exceptions here... So either it returns something, or it fails. Also unrelated, of course.


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@157 
PS2, Line 157: plmn
Hexstr


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@166 
PS2, Line 166: plmn
Hexstr


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@192 
PS2, Line 192: fivehexbytes:Hexstr
I still find it odd that we (ab)use strings to represent bytes in a language where we have a special type for that...


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@278 
PS2, Line 278: ->int
cosmetic: missing space


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@290 
PS2, Line 290: 	if imsi == None:
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.


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@298 
PS2, Line 298: long
bool


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@302 
PS2, Line 302: 	if imsi == None:
Same here.


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@350 
PS2, Line 350: ef_msisdn
Hexstr?


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@371 
PS2, Line 371: return None
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.


https://gerrit.osmocom.org/c/pysim/+/23594/2/pySim/utils.py@417 
PS2, Line 417: st
Hexstr? (not sure here, some magic happens on line 432)



-- 
To view, visit https://gerrit.osmocom.org/c/pysim/+/23594
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I50a0a07132890af0817f4ff0ce9fec53b7512522
Gerrit-Change-Number: 23594
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge at osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Comment-Date: Sat, 03 Apr 2021 23:33:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210403/31f40645/attachment.htm>


More information about the gerrit-log mailing list