7 Apr
2016
7 Apr
'16
11:15 a.m.
Thanks for review. Comments are inline
On 04/07/2016 12:47 AM, Neels Hofmeyr wrote:
A few things in this patch draw my attention:
- the verbose parameters have to be passed to each function call instead of
having a global verbosity switch? Can make sense, but does it? It doesn't
look like anyone is ever using the verbosity anyway?
They were useful for debugging the tests but once it's ready the'd just
clutter the output unnecessary. Still, they'll be useful again when we
want to expand tests further so instead of adding and removing them
every time I'd prefer to keep them.
- the function defs are below their use; not a problem in python but unusual as
far as I'm concerned.
Matter of taste I guess: I prefer to have all the functions not
belonging to particular class to be in one place instead being spread
over the file according to where it's used.
- 5 functions are added, only two are used. Why add unused functions?
Will they ever be used?
Perhaps :)
I've added them while trying to mock BSC and MSC. It turned out that not
all of them are actually necessary for current test-cases. Nevertheless
I'd rather keep them because those are simple wrappers around magic hex
constants for IPA protocol which are tried and tested, so next time we
need more tests we can just use them right away instead of duplicating
the efforts of finding out proper hex number sequences.
- Two functions are factored out and three others are added afresh. Admitted,
the second part of the log message mentions adding functions, yet the summary
above sounds like it's only factoring out.
How would you summarize it?
--
Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte