Attention is currently required from: neels. dexter 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: (14 comments) Patchset: PS6: I have looked through this now. Many of the comments are about cosmetic things. I think the only real problem I saw was that the get_next method of RandomHexDigitSource returns a byte array instead of hex digits. I would recommend to add a unit-test to be sure that the various value generators in param_source.py work as expected. File pySim/esim/saip/batch.py: https://gerrit.osmocom.org/c/pysim/+/41845/comment/8d49fed9_a5e0cf4d?usp=email : PS6, Line 65: params: list[ParamAndSrc]=None, why not use [] as default? ... https://gerrit.osmocom.org/c/pysim/+/41845/comment/e943de47_0f674ba4?usp=email : PS6, Line 80: self.params = params or [] ... then you could write self.params = params here. Maybe this is easier? https://gerrit.osmocom.org/c/pysim/+/41845/comment/e4dbb645_ad12fe16?usp=email : PS6, Line 92: csv_columns = next(self.csv_rows) In the above comment you say that self.csv_rows can also be a list. I think next() would not work on lists. Is the comment still up to date? or is something missing here? https://gerrit.osmocom.org/c/pysim/+/41845/comment/56c19288_ec4788f0?usp=email : PS6, Line 97: csv_row = None As far as I understand self.csv_rows is an optional parameter? From what I can see, the code should work fine as long as CsvSource is not used in this case. File pySim/esim/saip/param_source.py: https://gerrit.osmocom.org/c/pysim/+/41845/comment/f724aaf0_158e7a7f?usp=email : PS6, Line 31: class ParamSource: As far as I understand this is an abstract class. Maybe declare it as class ParamSource(abc.ABC) But when I look more closely, this is not really an abstract class. When I get you correctly you want users to be able to use this class as it is as a bare minimum. It wouldn't do anything useful, but it also wouldn't crash the application. https://gerrit.osmocom.org/c/pysim/+/41845/comment/587dd973_a74cc8f0?usp=email : 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 accept the string. https://gerrit.osmocom.org/c/pysim/+/41845/comment/4eb02f63_a84a2c51?usp=email : PS6, Line 51: Excess linebreak? (the other classes are only separated with one) https://gerrit.osmocom.org/c/pysim/+/41845/comment/58bb5fe7_8ecd4faf?usp=email : PS6, Line 65: def __init__(self, num_digits, first_value, last_value): Maybe add type annotations and agree on one distinct type? At the moment this function can accepts anything that can be casted to int. This is fine, but may also be a bit confusing. (see below) https://gerrit.osmocom.org/c/pysim/+/41845/comment/1bada13b_f050cff7?usp=email : PS6, Line 80: If this is a private/internal method, I would mark it with double underscore or single underscore? https://gerrit.osmocom.org/c/pysim/+/41845/comment/67ab85f0_e87a96f7?usp=email : 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. For the constructor this is ok (see above), but it may be confusing still. https://gerrit.osmocom.org/c/pysim/+/41845/comment/f9f5cb63_dc1aa1d8?usp=email : 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. Maybe return b2h(val) instead of val directly? https://gerrit.osmocom.org/c/pysim/+/41845/comment/b44471ae_26d9b7ba?usp=email : PS6, Line 149: if val > self.val_first_last[1]: self.last_value would be easier to read. https://gerrit.osmocom.org/c/pysim/+/41845/comment/12fdbdf0_b38d9d6d?usp=email : PS6, Line 174: return val You could just do if csv_row: return csv_row.get(self.csv_column) else: raise ...? -- To view, visit https://gerrit.osmocom.org/c/pysim/+/41845?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I01ae40a06605eb205bfb409189fcd2b3a128855a Gerrit-Change-Number: 41845 Gerrit-PatchSet: 6 Gerrit-Owner: neels <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-CC: dexter <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Comment-Date: Wed, 04 Mar 2026 16:51:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
