Patch Set 2: (2 comments)
I'd like you to do the small changes related to underscores. Other than that it looks good. https://gerrit.osmocom.org/#/c/2886/2/src/osmo_gsm_tester/bts_sysmo.py File src/osmo_gsm_tester/bts_sysmo.py: Line 93: log_ctx = proc > absolutely not double underscored, those are reserved for python internals. Well I was of course referring to the general case/mechanism of the log_ctx variable, not to this specific one. IMHO you can change the code which looks for that variable and use something more "outstanding". like _log_ctx_ or log_ctx__ should be enough. IMHO this already gives more hints to unaware reader that this variable may be "special" and worth looking for it in the code. All caps can be a good option too. I don't mind the exact way, but I'd like it to have something different from a regular local variable. https://gerrit.osmocom.org/#/c/2886/2/src/osmo_gsm_tester/process.py File src/osmo_gsm_tester/process.py: Line 77: self._set_name(self.name_str, pid=self.process_obj.pid) > a single underscore says: not supposed to use it directly. The idea was tha Then, if it's not supposed to be used directly don't use it, or change it so that it can be used (remove the underscore). If you think it's necessary you can add a comment saying why are you using it here too, but I don't think it's needed. -- To view, visit https://gerrit.osmocom.org/2886 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f9b53150f2bb6fa9d63ce27f0806f0ca6a45e90 Gerrit-PatchSet: 2 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-HasComments: Yes
