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=email : 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=email : 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=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. […] hm that's right, it has to be an iter(list), thx https://gerrit.osmocom.org/c/pysim/+/41845/comment/a3e376b3_476ec8e8?usp=email : 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=email : 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=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 […] 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=email : 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=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. […] heh funny code =) you are right https://gerrit.osmocom.org/c/pysim/+/41845/comment/c8a453fc_9b4a59b5?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. […] 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=email : 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 =) -- 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: dexter <[email protected]> Gerrit-Comment-Date: Tue, 10 Mar 2026 22:20:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: dexter <[email protected]>
