Attention is currently required from: neels.
14 comments:
Patchset:
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:
Patch Set #6, Line 65: params: list[ParamAndSrc]=None,
why not use [] as default? ...
Patch Set #6, Line 80: self.params = params or []
... then you could write self.params = params here. Maybe this is easier?
Patch Set #6, 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?
Patch Set #6, 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:
Patch Set #6, 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.
Patch Set #6, 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.
Excess linebreak? (the other classes are only separated with one)
Patch Set #6, 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)
If this is a private/internal method, I would mark it with double underscore or single underscore?
Patch Set #6, 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.
Patch Set #6, 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?
Patch Set #6, Line 149: if val > self.val_first_last[1]:
self.last_value would be easier to read.
Patch Set #6, Line 174: return val
You could just do if csv_row: return csv_row.get(self.csv_column) else: raise ...?
To view, visit change 41845. To unsubscribe, or for help writing mail filters, visit settings.