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

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Mon Dec 12 11:31:21 UTC 2016


Patch Set 3:

(4 comments)

It's a bit tiring to have to make the same explanations repeatedly.

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

Line 27: class IPA(object):
> Yes, defines.
our defines are always all-caps, but in function names we use all-lowercase. Your argument doesn't hold. I would still prefer "Ipa" to match the class naming scheme typical to python.


Line 48:         return 'UNKNOWN'
> So it's "potential errors due to typos" vs "potential errors due to unneces
I think I have made my point very clear. Potential errors due to typos are hard to find and go unseen for long periods of time. Returning None or raising an exception make it impossible to break the code in a hidden way.


Line 50:     def __tag(self, t, v):
> No, the convention is _ means "don't use unless you know what you're doing"
from above style guide link:

_single_leading_underscore : weak "internal use" indicator. E.g. from M import * does not import objects whose name starts with an underscore. 

__double_leading_underscore : when naming a class attribute, invokes name mangling (inside class FooBar, __boo becomes _FooBar__boo ; see below). 

Generally, double leading underscores should be used only to avoid name conflicts with attributes in classes designed to be subclassed. 

Use a single underscore and be done with it.


Line 113:         return self.__tag(self.IDTAG['SERNR'], data)
> I don't get the question: they are defined in a central place - it's right 
above do
  SERNR = 'SERNR'
and everywhere else use the constant instead of the string to avoid hidden-typo bugs. A general practice to improve code maintainability.


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