Attention is currently required from: laforge.
dexter has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/pysim/+/38195?usp=email )
Change subject: filesystem: pass total_len to construct of when encoding file contents ......................................................................
Patch Set 7:
(6 comments)
Patchset:
PS6:
I think what we see from the several last few iterations of this patchset is that there is a lack of […]
I also think that we should have some specific unittests. I have now created a comprehensive unittest that verifies the encoding of all three file types in all its variants. The tests also verify if total_len is correctly passed. I also verified that the tests detect bugs that were overlooked in the current patchset.
File pySim/filesystem.py:
https://gerrit.osmocom.org/c/pysim/+/38195/comment/a660f0e0_3c42b118?usp=ema... : PS6, Line 746: def __get_size(self, total_len: Optional[int] = None) -> int:
must be `-> Optional[int]` as it also may return `None`
Done
https://gerrit.osmocom.org/c/pysim/+/38195/comment/30d0f39a_22249f27?usp=ema... : PS6, Line 1054: def __get_rec_len(self, total_len: Optional[int] = None) -> int:
likewise: `-> Optional[int]`
Done
https://gerrit.osmocom.org/c/pysim/+/38195/comment/9ba0839d_88d56015?usp=ema... : PS6, Line 1099: return b2h(t.to_tlv()) I think we have overlooked files that use _tlv. Shouldn't padd to total_len here?
https://gerrit.osmocom.org/c/pysim/+/38195/comment/7d53f8fa_27aaf2e0?usp=ema... : PS6, Line 1274:
here we still don't return an Optional[int] but a dict? I think that's inconsistent comparing the _ […]
Done
https://gerrit.osmocom.org/c/pysim/+/38195/comment/2ebf1bfe_7586fcdc?usp=ema... : PS6, Line 1303:
and this will again break. […]
Done