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/.
Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Patch Set 2: Code-Review-1
(9 comments)
A few stuff to be fixed. Good job, happy to get rid of the "with" statements :D
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 look like it's gonna make sense for 1000 next people reading this code :P
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?
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 at first glance it seems it's a foobar variable which was left here being unused at some point in time, and someone may be willing to remove them.
__log_ctx__ ?
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 ?
Line 316: raise RuntimeError('No IMSI')
log.Error?
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?
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
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 callers in some way? We can directly do the _src=4 below.
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.
How are tests calling log now? Or they should only call print() ?
--
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: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-HasComments: Yes