This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Patch Set 3: Code-Review-1
(14 comments)
Mostly style comments, feel free to disagree
https://gerrit.osmocom.org/#/c/1265/3/openbsc/contrib/ipa.py
File openbsc/contrib/ipa.py:
Line 27: class IPA(object):
IMvHO despite being an acronym, the class name should avoid all-caps
Line 29: Stateless IPA protocol multiplexor: add/remove/parse (extended) header
IMHO the spelling should be "multiplexer" with "er"
Line 35: PROTO = dict(RSL = 0x00, CCM = 0xFE, SCCP = 0xFD, OML = 0xFF, OSMO = 0xEE, MGCP_OLD = 0xFC)
typically in named args to function calls, there are no spaces around the '='
Line 45: """
A single line docstring like this could stay on a single line, especially if the function is short. Same numerous times below.
def f():
"single line comment"
code()
Line 48: return 'UNKNOWN'
I think better would be to return None or raise an exception.
If it needs to be a constant, rather define in a global var.
Also use 'is' for 'None', and also early exit:
if p is None:
return None
return list(...
Line 50: def __tag(self, t, v):
any particular reason for *double* underscore?
Line 94: if ext != None:
'if ext is not None:'
Line 113: return self.__tag(self.IDTAG['SERNR'], data)
I think it would be good to define the IDTAG keys as constants, like the TCP_PORT_OML above, to avoid typo errors in calling code. (Only worth it if there will actually be calling code hardcoding these names.)
Line 163: def identity(self, unit = b'', mac = b'', location = b'', utype = b'', equip = b'', sw = b'', name = b'', serial = b''):
here I'd also drop the spaces around '='.
Line 229: if 'ERROR' == s:
you like yoda conditionals :)
but they have no benefit in python.
I don't like how a hardcoded string is used to make decisions, it should be a string constant defined once.
Line 236: if op != None:
if op is not None:
Line 249: def cmd(self, var, val = None):
val=None
Line 263: if k != var or (val != None and v != val):
'val is not None'
Line 268: print("IPA multiplexor v%s loaded." % IPA.version)
multiplexer
--
To view, visit https://gerrit.osmocom.org/1265
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I41e37ec143183e422c0b31af95d183bd84f0c328
Gerrit-PatchSet: 3
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes