Attention is currently required from: laforge.
neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/pysim/+/40096?usp=email )
Change subject: personalization: add param_source.py, implement batch personalization ......................................................................
Patch Set 10:
(4 comments)
Patchset:
PS6:
I think we have had this discussion in the osmocom community before, and the general social consensu […]
In simple patches this makes sense, but there is a bound to this:
As a developer I need to be able to *use* the infrastructure that we have instead of running my laptop hot, also considering that i cannot run the complete pysim test suite on my laptop to begin with.
With a sufficiently complex branch, trying to be considerate about what to push becomes unfeasible.
When the gerrit job takes >10 minutes to finish each commit, it must be allowed to push ahead and not have to wait on each patch individually.
IMHO we have the [x] Resolved / DONE markings to indicate patch status for reviewers.
File pySim/esim/saip/param_source.py:
https://gerrit.osmocom.org/c/pysim/+/40096/comment/4c1deccc_ed242473?usp=ema... : PS10, Line 50:
This is dead code. […]
ok, then yet another ABC cycle.
Ack, with that useless @abstractmethod decorator it is indeed dead code.
We clearly have different approaches. In my opinion, *ABC* is the dead code, and you keep advocating it besides the points I make against it -- I have many points against it and I find them logical and simple.
Not least that I have spent days exploring the world of ABC, found mostly problems it creates, and *one* valid point for it: that developers can read in the code that there is inheritance, which a single word comment can do likewise.
So I would very much enjoy if ABC took far less space and time in CR from now on.
https://gerrit.osmocom.org/c/pysim/+/40096/comment/059a7954_db20a739?usp=ema... : PS10, Line 84:
likewise here, just `pass` will do.
Done
File pySim/esim/saip/personalization.py:
https://gerrit.osmocom.org/c/pysim/+/40096/comment/af6ada37_3f165569?usp=ema... : PS10, Line 705: def __init__(self,
highly unusual formatting which we don't do this way in any other part o the code. […]
I am trying hard to read this CR as neutral fact. Thank you for a considerate choice of words in CR comments.
I find this formatting readable, because the type annotations clutter up the function signature.
For important styling, we should have a linter.
For perspective, I have recently seen a pysim patch with weird spaces in the function signature, and including an obvious bug, being +2 and merged within minutes. So, what I mean is, I could use any help to get this branch merged.