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 2:
(9 comments)
https://gerrit.osmocom.org/#/c/2886/2/selftest/log_test.py
File selftest/log_test.py:
Line 89: # some space to keep source line numbers identical to previous code
> This can be dropped right? it may make sense for this review but doesn't lo
it makes sense to justify why I added three blank lines. I want the regression test changes of this patch to stand alone, we can drop this line, change the whitespace and adjust line numbers in a subsequent patch.
https://gerrit.osmocom.org/#/c/2886/2/selftest/suite_test.py
File selftest/suite_test.py:
Line 23: trial = log.Origin(None, 'trial')
> worth it seeting this first param as kwargs which defaults to None?
The only reason to have this None is to keep test output unchanged --> follow up patch
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
> Can we make this variable have a name which looks more special? otherwise a
absolutely not double underscored, those are reserved for python internals.
A single underscore says "please don't use directly", but we want this used.
All-caps?
IMHO we just have to know that log_ctx is logging context?
I know it's non-pythonic in the sense of "explicit is better than implicit" ... but we can't use a set_log_ctx() function because that would not be tied to this code frame.
Using 'self' is just as implicit.
No underscore or different naming will change the implicit magic nature... do you have any idea how to make log_ctx more explicit and less magic?
(this particular instance is straightforward, but I mean the general case of finding any 'log_ctx' in any parent scope of an exception raised)
https://gerrit.osmocom.org/#/c/2886/2/src/osmo_gsm_tester/ofono_client.py
File src/osmo_gsm_tester/ofono_client.py:
Line 123: raise RuntimeError('Modem interface is not available:', interface_name)
> log.Error ?
(also works with RuntimeError, but indeed for message composition, thx)
Line 316: raise RuntimeError('No IMSI')
> log.Error?
(in this case it doesn't matter at all, but ok)
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)
> why have an underscore now?
a single underscore says: not supposed to use it directly. The idea was that everyone should rather use the constructor to set the name. In this instance, we know the pid only later.
Mostly I added the underscore to make sure I have no code left that uses the old set_name() instead of the constructor. I could change it back to no-underscore, but I thought it's good to give an incentive not to forget calling the constructor.
https://gerrit.osmocom.org/#/c/2886/2/src/osmo_gsm_tester/suite.py
File src/osmo_gsm_tester/suite.py:
Line 82: self.suite_run = suite_run
> No need to move suite_run around in here, but not a big issue
yea, I want the super()'s constructor to be called as early as possible, and self.basename is the only thing needed above it.
I could remove the 'self' but want to semantically emphasize that it's the same as self.basename
Line 128: def set_fail(self, fail_type, fail_message, tb_str=None, src=4):
> Do we really need to pass this over here? Is it expected to be changed by c
In case of reporting an exception, we want to pass the exception's src instead, in suite.py.
https://gerrit.osmocom.org/#/c/2886/2/src/osmo_gsm_tester/test.py
File src/osmo_gsm_tester/test.py:
Line 35: global trial, suite, test, resources, log, dbg, err, wait, wait_no_raise, sleep, poll, prompt, Timeout
> Missed removing them from this line too.
yes, my intention was to only call print(). Of course we don't have a dbg log then. Strictly speaking this change is unrelated.
--
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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-HasComments: Yes