openbsc[master]: Add IPA multiplex

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/.

Max gerrit-no-reply at lists.osmocom.org
Mon Dec 12 09:40:25 UTC 2016


Patch Set 3:

(6 comments)

Did I got it right that the only disagreement right now is of purely aesthetically nature? In the absence of Osmocom coding style guide for python I don't think it warrants -1 review.

https://gerrit.osmocom.org/#/c/1265/3/openbsc/contrib/ipa.py
File openbsc/contrib/ipa.py:

Line 27: class IPA(object):
> which names? #defines? because all our function prefixes also avoid all-cap
Yes, defines.


Line 35:     PROTO = dict(RSL = 0x00, CCM = 0xFE, SCCP = 0xFD, OML = 0xFF, OSMO = 0xEE, MGCP_OLD = 0xFC)
> Not at all. In all python code I've seen, kwargs have no spaces. If you wri
It looks like assignment because it is assignment. I don't get what's the confusion about?


Line 45:         """
> lol "how many LIES" :)
Lol indeed, perfect typo when it comes to documentation :) Nevertheless, if documentation for a function looks "bloated" compared to function body I don't consider it a bad thing: we're not hard-pressed to squeeze SLOC to an absolute minimum - certainly not at the cost of readability.


Line 48:         return 'UNKNOWN'
> The benefit is hugely obvious. If you ever compare the return value, you ca
So it's "potential errors due to typos" vs "potential errors due to unnecessary complex dispatch code". My heart goes for the latter: typos seems to be easier to catch.


Line 50:     def __tag(self, t, v):
> double underscores are typically for python language constructs like __doc_
No, the convention is _ means "don't use unless you know what you're doing", while __ means "private, never use directly". In this case the semantic is the latter, changing underscores would send the wrong message the the users of this class.


Line 113:         return self.__tag(self.IDTAG['SERNR'], data)
> Sorry, I don't follow. Why do you not want to define constants in a central
I don't get the question: they are defined in a central place - it's right on top of this file. Could you elaborate?


-- 
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: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list