Attention is currently required from: dexter.

neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/pysim/+/41845?usp=email )

Change subject: personalization: add param_source.py, add batch.py ......................................................................

Patch Set 6:

(11 comments)

Patchset:

PS6: there will be a new patch set soon, thanks for the feedback!

File pySim/esim/saip/batch.py:

https://gerrit.osmocom.org/c/pysim/+/41845/comment/f24a7bae_34d7aa7a?usp=ema... : PS6, Line 65: params: list[ParamAndSrc]=None,

why not use [] as default? ...

IIRC the linter warns against passing empty lists as default argument, because in python there can arise an evil twin situation that is hard to see: like some functions change the arg in-place or something, i don't remember in detail.

https://gerrit.osmocom.org/c/pysim/+/41845/comment/bd98a8ed_3caf0795?usp=ema... : PS6, Line 80: self.params = params or []

... then you could write self.params = params here. […]

(marking done, bc discussion will be above)

https://gerrit.osmocom.org/c/pysim/+/41845/comment/32c6d044_21645ff6?usp=ema... : PS6, Line 92: csv_columns = next(self.csv_rows)

In the above comment you say that self.csv_rows can also be a list. […]

hm that's right, it has to be an iter(list), thx

https://gerrit.osmocom.org/c/pysim/+/41845/comment/a3e376b3_476ec8e8?usp=ema... : PS6, Line 97: csv_row = None

As far as I understand self. […]

it is optional, configured by the caller, whether a CSV file is present. If yes, it will pass nonempty csv_rows and use at least one CsvSource. So the csv_rows != None case definitely has to be supported.

File pySim/esim/saip/param_source.py:

https://gerrit.osmocom.org/c/pysim/+/41845/comment/df80bc66_187a2e6c?usp=ema... : PS6, Line 31: class ParamSource:

As far as I understand this is an abstract class. Maybe declare it as […]

That is right. The ABC discussion is a slightly dangerous one, because I am pretty firm in my opinion against using it, though not everyone seems to agree with it. It would be great if I am allowed to work without ABC, or otherwise let's schedule a call, or i will hold a talk about ABC -- either way let's not list points for and against it here but rather talk in person?

https://gerrit.osmocom.org/c/pysim/+/41845/comment/b655dbde_252b85c9?usp=ema... : PS6, Line 42: so the user can enter '0000' to get a four digit random number."""

If this should work as the bare minimum, we should also have some kind of dummy constructor that can […]

Hmm. it's even worse, most subclasses completely replace this. I will make a better plan, thanks.

https://gerrit.osmocom.org/c/pysim/+/41845/comment/4cd391b0_8a0ba5dd?usp=ema... : PS6, Line 51:

Excess linebreak? (the other classes are only separated with one)

yea you're right, sometimes i do sometimes i don't, thx.

i think it's because this separates the base definitions from the actual implementations, but not important

https://gerrit.osmocom.org/c/pysim/+/41845/comment/944bfb95_3efd540d?usp=ema... : PS6, Line 95: last_value = int(last_str) if last_str is not None else "9" * len(first_str)

Here last_value is either an int or it is a string with an integer number. […]

heh funny code =) you are right

https://gerrit.osmocom.org/c/pysim/+/41845/comment/c8a453fc_9b4a59b5?usp=ema... : PS6, Line 121: val = random.randbytes(self.num_digits // 2) # TODO secure random source?

As far as I understand this should return a bytearray but what you actually want is a hexstring. […]

This uses the fact that all BinaryParameter can take a bytes as input value. We could convert to hexstring and then the BinaryParameter can convert back to binary, but by passing just the bytes we skip the two conversions.

A thought could be this: most places now use a string value to pass parameter settings around, so if we define that a string is the preferred way, then i would change this; but the idea in ConfigurableParameter that I took over from the code I got when I started is that a bytes and BytesIO and hexstring all work, so i would leave the patch unchanged.. agreed?

https://gerrit.osmocom.org/c/pysim/+/41845/comment/3a1f2a8f_e471a54c?usp=ema... : PS6, Line 174: return val

You could just do if csv_row: return csv_row.get(self.csv_column) else: raise ... […]

it's more like

if not csv_row or not csv_row.get(self.csv_column): raise... return csv_row.get(self.csv_column)

so i'm pretty sure this patch as it is is the most minimalistic way of writing this =)