Attention is currently required from: dexter.
11 comments:
Patchset:
there will be a new patch set soon, thanks for the feedback!
File pySim/esim/saip/batch.py:
Patch Set #6, 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.
Patch Set #6, Line 80: self.params = params or []
... then you could write self.params = params here. […]
(marking done, bc discussion will be above)
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. […]
hm that's right, it has to be an iter(list), thx
Patch Set #6, 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:
Patch Set #6, 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?
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 […]
Hmm. it's even worse, most subclasses completely replace this. I will make a better plan, thanks.
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
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. […]
heh funny code =) you are right
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. […]
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?
Patch Set #6, 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 change 41845. To unsubscribe, or for help writing mail filters, visit settings.