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.orgPatch Set 9: Code-Review-1 (7 comments) I do uphold most of my comments which you seem to disagree with. Sorry that I commented on PS3 instead of the last PS, but the comments are still valid AFAICT https://gerrit.osmocom.org/#/c/1265/3/openbsc/contrib/ipa.py File openbsc/contrib/ipa.py: Line 27: class IPA(object): > I think it's better to stick to the same names as in libosmocore. which names? #defines? because all our function prefixes also avoid all-caps. Line 35: PROTO = dict(RSL = 0x00, CCM = 0xFE, SCCP = 0xFD, OML = 0xFF, OSMO = 0xEE, MGCP_OLD = 0xFC) > This would decrease readability without providing any obvious benefits. Not at all. In all python code I've seen, kwargs have no spaces. If you write with spaces, it looks like assignments to me: confusing. Line 45: CTRL_TRAP = 'TRAP' > I think it's better to use the same style for docstrings regardless of how lol "how many LIES" :) Come on, this is code bloat that outweighs the function body. Line 48: """ > This would make dispatcher code which uses it unnecessary hard without prov The benefit is hugely obvious. If you ever compare the return value, you can introduce typos without noticing. The None construct exists for exactly these cases. Line 50: """ > that's python syntax for private functions. double underscores are typically for python language constructs like __doc__ or __name__. Please use single underscore instead. Line 113: if not len(data): > Calling code is expected to work with tag_* functions. Sorry, I don't follow. Why do you not want to define constants in a central place? Line 163: def tag_ip(self, data): > why? it's easier to read when spaces are there. see above, kwargs convention -- 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: 9 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